[workers-auth] Extract auth into @cloudflare/workers-auth, make it product-agnostic, and add a cf entrypoint#14630
Open
penalosa wants to merge 8 commits into
Open
[workers-auth] Extract auth into @cloudflare/workers-auth, make it product-agnostic, and add a cf entrypoint#14630penalosa wants to merge 8 commits into
penalosa wants to merge 8 commits into
Conversation
…xport shims Move Wrangler's auth machinery into the @cloudflare/workers-auth `/wrangler` layer behind a createWranglerAuth(ctx) factory whose context is reduced to 5 fields (logger, userAgent, prompt, select, isNoDefaultValueProvidedError). config-cache, isInteractive and openInBrowser move to @cloudflare/workers-utils (logger passed as a param, no singleton there). Delete the re-export/adapter shim files that only existed to preserve historical import paths (preferences, auth-config-file, profile-store, keyring-preference, generate-auth-url, is-interactive, open-in-browser) and repoint every consumer to the owning package. Slim auth-variables to its single genuine export (getWranglerR2SqlAuthToken). Login-URL snapshot determinism is handled by scrubbing the OAuth state / code_challenge params in workers-utils normalizeString rather than mocking the URL builder.
🦋 Changeset detectedLatest commit: ff57a0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
Contributor
|
The I've now completed a thorough review of the core logic paths, deleted-file consumers, test mocking, cross-package live bindings, function signatures, env var behavior, and telemetry messages. This is a well-executed, faithful internal refactor. The main risks (credential resolution path, paged fetch, scope live binding, config-cache pointing at same files, browser-open logger param) all check out as behavior-preserving. I did not find any logic bugs, security issues, backward-compatibility violations, or incorrect API behavior. The only observations are stylistic (split imports from the same module) which Prettier/ESLint handle and I'm instructed not to flag. LGTM |
Importing `open` at module top pulled its default `node:child_process` import into every consumer of the workers-utils barrel. Tests that partially mock `node:child_process` (e.g. workers-auth's access.test.ts, with only `spawnSync` and no default export) then failed at import time. Import `open` lazily inside openInBrowser so merely importing the barrel no longer drags child_process into the module graph.
…ed product-agnostic core Refactor the wrangler-specific auth layer into a product-agnostic `createCloudflareAuth(product, ctx)` core (`src/product/`) parameterised by an `AuthProduct` descriptor (identity, config paths, file format, consent pages, client id, copy). Wrangler becomes a thin descriptor (`WRANGLER_PRODUCT` + `createWranglerAuth`) with its `/wrangler` entrypoint exports unchanged. Add a new `@cloudflare/workers-auth/cf` entrypoint (`createCfAuth`): the same auth machinery, but JSON files under `~/.config/cloudflare`, the `cloudflare` OS-keyring service, and `CLOUDFLARE_CLIENT_ID`. The on-disk format is a new `FileFormat` axis threaded through the credential store (default TOML, so wrangler is byte-for-byte unchanged). cf's OAuth app client-id UUIDs, redirect URI, consent page URLs, and the account-selection config-file hint are placeholders pending real values (search TODO(cf)).
workers-devprod
requested review from
a team and
dario-piotrowicz
and removed request for
a team
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…er tests Moving isInteractive/isNonInteractiveOrCI into workers-utils (and reading CI in workers-auth's account-name redaction) meant CI detection now runs inside packages that wrangler consumes as bundled distributables. The inlined ci-info copy is a separate, unmockable module instance that reads the real environment, so in CI (isCI=true) every confirm/select prompt silently auto-answered non-interactively — breaking ~460 wrangler tests — and the account-name redaction check ran against the real CI flag instead of the mocked one, so its test had lost coverage. - workers-utils: add an isCI() helper alongside isInteractive/ isNonInteractiveOrCI. - workers-auth: read isCI() from workers-utils in the account-selection redaction path instead of importing ci-info directly (drops workers-auth's ci-info dependency). - wrangler tests: extend the existing @cloudflare/workers-utils partial-mock to also route isNonInteractiveOrCI/isCI through the mockable ci-info. This reaches every consumer — wrangler source and the bundled workers-auth (@cloudflare/* is external in its bundle) — so the existing vi.mocked(ci).isCI convention keeps controlling CI-gated behaviour, and the redaction test again exercises the real code path (snapshot restored to "(redacted)").
…te cf's config cache - hasEnvCredentials now uses the product's allowGlobalAuthKey, so cf (scoped-token-only) no longer treats a bare CLOUDFLARE_API_KEY+CLOUDFLARE_EMAIL pair as "logged in via env" and wrongly blocks login/logout. - createConfigCache takes a namespace so each CLI gets an isolated cache dir; cf uses "cloudflare" so cf login/logout never purges wrangler's cache. WRANGLER_CACHE_DIR stays wrangler-only. - Add regression tests + a changeset for the workers-auth/cf entrypoint.
Contributor
|
✅ All changesets look good |
penalosa
changed the title
5 tasks
emily-shen
reviewed
Jul 10, 2026
Contributor
There was a problem hiding this comment.
i suppose we don't have to do this now, but we can remove isNonInteractiveOrCI from the deploy-heleprs context entirely and just import directly from workers-utils
| @@ -0,0 +1,69 @@ | |||
| // The Cloudflare OAuth scope catalog, shared by every CLI built on this auth | |||
| // layer (wrangler, cf, …). `DefaultScopeKeys` is a mutable live binding: | |||
| // `setLoginScopeKeys` reassigns it (wrangler's `--experimental-scopes` / product | |||
Contributor
There was a problem hiding this comment.
what is --experimental-scopes? is it new in this PR? can we get a changeset for it?
Contributor
There was a problem hiding this comment.
can we rename this folder 'core'? i was confused by what product meant here (its the non-product specific parts?)
Contributor
There was a problem hiding this comment.
other than the interfaces, is this all just moved from other places? (do i need to review this?)
| @@ -0,0 +1,77 @@ | |||
| import { | |||
| getAuthConfigFilePath as getAuthConfigFilePathForConfig, | |||
Contributor
There was a problem hiding this comment.
Suggested change
| getAuthConfigFilePath as getAuthConfigFilePathForConfig, | |
| getAuthConfigFilePath as getAuthConfigFilePathInternal, |
something like that? getAuthConfigFilePathForConfig is a bit convoluted 😅
Comment on lines
+14
to
+28
| * A file-on-disk storage backend, parameterised by the on-disk {@link FileFormat} | ||
| * (TOML for wrangler, JSON for cf) and the path it reads and writes. Used by the | ||
| * temporary-preview-account store and the plaintext auth-profile primitives. | ||
| * | ||
| * `read()` follows the `ConfigStorage<T>` contract: a missing file *or* a file | ||
| * that exists but can't be parsed as `T` is the empty state and returns | ||
| * `undefined`, so a corrupt store is treated as "nothing usable here" (e.g. a | ||
| * stale temporary-account cache is re-minted instead of hard-erroring). Only | ||
| * genuine I/O errors (`EACCES`, `EISDIR`, ...) propagate. This is deliberately | ||
| * distinct from `FileCredentialStore`, which surfaces a corrupt *credential* | ||
| * file as a throw so the user can fix it. | ||
| * | ||
| * Files are written with mode `0o600` on creation and re-`chmod`'d on every | ||
| * save (the `mode` option only applies on creation) so other local users on | ||
| * shared hosts can't read the stored credentials. |
Contributor
There was a problem hiding this comment.
Suggested change
| * A file-on-disk storage backend, parameterised by the on-disk {@link FileFormat} | |
| * (TOML for wrangler, JSON for cf) and the path it reads and writes. Used by the | |
| * temporary-preview-account store and the plaintext auth-profile primitives. | |
| * | |
| * `read()` follows the `ConfigStorage<T>` contract: a missing file *or* a file | |
| * that exists but can't be parsed as `T` is the empty state and returns | |
| * `undefined`, so a corrupt store is treated as "nothing usable here" (e.g. a | |
| * stale temporary-account cache is re-minted instead of hard-erroring). Only | |
| * genuine I/O errors (`EACCES`, `EISDIR`, ...) propagate. This is deliberately | |
| * distinct from `FileCredentialStore`, which surfaces a corrupt *credential* | |
| * file as a throw so the user can fix it. | |
| * | |
| * Files are written with mode `0o600` on creation and re-`chmod`'d on every | |
| * save (the `mode` option only applies on creation) so other local users on | |
| * shared hosts can't read the stored credentials. | |
| * adapter for writing TOML or JSON files |
| * `CLOUDFLARE_ACCOUNT_ID` overrides the account inferred from the current user. | ||
| * | ||
| * This is a Cloudflare-wide variable (not wrangler-specific), so it lives in the | ||
| * shared core rather than a consumer layer. `CF_ACCOUNT_ID` is the deprecated |
Contributor
There was a problem hiding this comment.
the rest of this file is mostly 'WRANGLER_' env vars, which should probably be included in the core product/cf/wrangler refactor?
Contributor
There was a problem hiding this comment.
what about moving this into product/, and just making wrangler/cf set fileFormat: 'toml' | 'json' instead of passing in these objects? it's not really extensible as it is, not do we really want to add more file types
| // CLOUDFLARE_EMAIL pair (with no CLOUDFLARE_API_TOKEN) must NOT count as | ||
| // "logged in via env". Otherwise logout/login would wrongly bail with | ||
| // "You are logged in with an API Token" even though cf ignores those creds. | ||
| vi.stubEnv("CLOUDFLARE_API_KEY", "deadbeef"); |
| * the same regardless of `WRANGLER_API_ENVIRONMENT`; override via the env var | ||
| * if a different app is ever needed. | ||
| */ | ||
| export const getClientIdFromEnv = getEnvironmentVariableFactory({ |
Contributor
There was a problem hiding this comment.
is there any point in having this as an env var and not just hard coding it in?
'CLOUDFLARE_CLIENT_ID' feels like a very generic non-auth specific env var that might be useful for something else at some point in time
Contributor
Author
There was a problem hiding this comment.
We use WRANGLER_CIENT_ID for this in Wrangler, so this is trying to mirror that
Contributor
There was a problem hiding this comment.
is there any point in that either, really? it wouldn't be breaking to remove it, given no one external can generate these ids. or we could rename it WRANGLER_OAUTH_CLIENT_ID as well
Contributor
There was a problem hiding this comment.
i'm not dying on this hill though, feel free to resolve
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor Wrangler's auth machinery into
@cloudflare/workers-auth, make the CLI-facing auth layer product-agnostic, and add a second consumer (/cf) on top of the shared core.What changed
@cloudflare/workers-authbehindcreateWranglerAuth(ctx)(5-field context:logger,userAgent,prompt,select,isNoDefaultValueProvidedError). Everything previously injected is built in-package: config-cache, the temporary-terms prompt, and paged account fetching (fetchAccountsPaged, avoiding thecfetch↔usercycle). The re-export/adapter shim files (preferences,auth-config-file,profile-store,keyring-preference,generate-auth-url,is-interactive,open-in-browser) are deleted and every consumer repointed at the owning package.src/product/) is parameterised by anAuthProductdescriptor (client ID, consent pages, config paths, file format, scope catalog, cache namespace, …), so each CLI is a thin descriptor + entrypoint rather than a fork of the factory.@cloudflare/workers-auth/cfentrypoint —createCfAuth(ctx)for thecfCLI, mirroring its real OAuth app registration: client ID, local callback port 8877, brandedcf-oauth-consent-*pages, scoped-token-only auth (no global API key), JSON config files under~/.config/cloudflare, a dedicated cf scope catalog, and an isolated config-cache namespace.createConfigCache,isInteractive/isNonInteractiveOrCI/isCI, andopenInBrowsermoved to@cloudflare/workers-utils(logger is passed as a parameter — there is no logger singleton there).ci-info, so consumers mock@cloudflare/workers-utils' CI helpers (notci-info) to fake CI. Wrangler's vitest setup does this, and because@cloudflare/*is external in the workers-auth bundle the seam reaches the bundled auth layer too.hasEnvCredentialshonours the product'sallowGlobalAuthKey(so scoped-token-only cf isn't wrongly blocked by a bareCLOUDFLARE_API_KEY+CLOUDFLARE_EMAILpair); cf's config cache is namespaced socf login/logoutnever purges Wrangler's cache; cf's config dir has no leading dot.state/code_challengequery params inworkers-utilsnormalizeString, rather than mocking the URL builder.No user-facing Wrangler behaviour change.
@cloudflare/workers-authgains a new internal/cfentrypoint (covered by the changeset).@cloudflare/workers-authis an internal package with no public documentation.