[wrangler] Only show accounts available for the current login auth#13770
[wrangler] Only show accounts available for the current login auth#13770petebacondarwin merged 12 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 89b895e The changes in this PR will be included in the next version bump. 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 |
|
Good. When After thoroughly reviewing the PR, I'm satisfied that the logic is correct and well-tested. The implementation handles all edge cases properly:
The test coverage is comprehensive, covering intersection, filtering in both directions, 9109 fallback, permissive mode, and the The changeset is properly formatted and classified as a patch. LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
✅ All changesets look good |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
abfc494 to
f28d9a9
Compare
60cfedc to
06d382c
Compare
|
@penalosa - this will probably need a "Codeowners Bypass" review since it touches D1 and KV tests. |
c3a97ae to
29e9093
Compare
penalosa
left a comment
There was a problem hiding this comment.
Some nits, but otherwise lgtm
Wrangler now intersects `/accounts` and `/memberships` when listing accounts in `wrangler whoami` and the interactive 'Select an account' prompt. Previously `whoami` rendered every account the underlying user belonged to, including accounts the active OAuth token or API token had no membership in, and it could disagree with the account picker (which only consulted `/memberships`). For Account API Tokens (where `/memberships` returns 9109) the behavior is unchanged: the single account associated with the token is shown via the existing `/accounts` fallback.
Per review feedback, drop the broad permissive escape hatch in fetchAllAccounts and only fall back to /accounts when /memberships fails with 9109 (Insufficient permissions) — the structural Account API Token case. Any other failure on either endpoint is now surfaced to the user instead of silently degrading. This also removes the /accounts-failure fallback to /memberships: if /accounts is unavailable we cannot compute the intersection correctly, so we propagate the error. whoami no longer requests permissive mode and will now fail when /memberships is broken, matching the rest of the CLI.
Per follow-up review, broaden the /memberships fallback so that both 9109 (Insufficient permissions) and 10000 (Authentication error) trigger the fall back to /accounts. 10000 covers tokens that work against /accounts but not /memberships (e.g. missing the membership read scope), so callers shouldn't be blocked by it. The fallback is now the default for all callers — whoami, account selection, and any other code path through fetchAllAccounts — removing the previous integration-test-only special case. Restore the unit test that asserts the 10000 fallback works, and add a matching 'helpful error when /accounts is also unusable' test for parity with the 9109 case. Update the propagate test to use a code outside the tolerated set (1003), and restore the whoami snapshot test for 10000 alongside a new whoami test that asserts non-tolerated /memberships errors still fail the command.
29e9093 to
be0c4ec
Compare
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Wrangler's account listings now respect the active login auth:
wrangler whoamiand the interactive 'Select an account' prompt show the intersection of/accountsand/memberships, dropping any accounts the current OAuth token or API token has no membership in.Previously these two code paths used different sources:
wrangler whoamilisted every account fromGET /accounts— including ones the active token couldn't actually use.Select an accountprompt (and the non-interactive 'no account ID' error message) listed accounts fromGET /memberships.That could produce a
whoamitable where an account showed up but couldn't be picked from the prompt or used by other commands. After this change both flows sharefetchAllAccounts, which:/accountsand/membershipsin parallel./accounts./accountswhen/membershipsreturns 9109 (Insufficient permissions) or 10000 (Authentication error). 9109 is the structural Account API Token case; 10000 covers tokens that work against/accountsbut are not accepted by/memberships. Any other failure on either endpoint is surfaced to the user.throwOnEmptyflag sowhoamishows an empty list rather than throwing when the intersection is empty, while account selection still throws (as before) so the error guides the user to setaccount_id.The
accountsfield ofwrangler whoami --jsonis more restrictive in the same way.wrangler whoamiand account-selection UX are unchanged on the surface aside from the filtered list.