diff --git a/.changeset/hungry-ways-boil.md b/.changeset/hungry-ways-boil.md new file mode 100644 index 000000000000..850646cd3f16 --- /dev/null +++ b/.changeset/hungry-ways-boil.md @@ -0,0 +1,12 @@ +--- +"wrangler": patch +--- + +fix: stop wrangler spamming console after login + +If a user hasn't logged in and then they run a command that needs a login they'll get bounced to the login flow. +The login flow (if completed) would write their shiny new OAuth2 credentials to disk, but wouldn't reload the +in-memory state. This led to issues like #693, where even though the user was logged in on-disk, wrangler +wouldn't be aware of it. + +We now update the in-memory login state each time new credentials are written to disk. diff --git a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts new file mode 100644 index 000000000000..7ec9e1b0047d --- /dev/null +++ b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts @@ -0,0 +1,178 @@ +import { ChildProcess } from "child_process"; +import fetchMock from "jest-fetch-mock"; +import { Request } from "undici"; +const { fetch } = jest.requireActual("undici") as { + fetch: (input: string) => Promise; +}; + +// the response to send when wrangler wants an oauth grant +let oauthGrantResponse: GrantResponseOptions | "timeout" = {}; + +/** + * A mock implementation for `openInBrowser` that sends an oauth grant response + * to wrangler. Use it like this: + * + * ```js + * jest.mock("../open-in-browser"); + * (openInBrowser as jest.Mock).mockImplementation(mockOpenInBrowser); + * ``` + */ +export const mockOpenInBrowser = async (url: string, ..._args: unknown[]) => { + const { searchParams } = new URL(url); + if (oauthGrantResponse === "timeout") { + throw "unimplemented"; + } else { + const queryParams = toQueryParams(oauthGrantResponse, searchParams); + // don't await this -- it will block the rest of the login flow + fetch(`${searchParams.get("redirect_uri")}?${queryParams}`).catch((e) => { + throw new Error( + "Failed to send OAuth Grant to wrangler, maybe the server was closed?", + e as Error + ); + }); + return new ChildProcess(); + } +}; + +/** + * Functions to help with mocking various parts of the OAuth Flow + */ +export const mockOAuthFlow = () => { + afterEach(() => { + fetchMock.resetMocks(); + }); + + const mockGrantAuthorization = ({ + respondWith, + }: { + respondWith: "timeout" | "success" | "failure" | GrantResponseOptions; + }) => { + if (respondWith === "failure") { + oauthGrantResponse = { + error: "access_denied", + }; + } else if (respondWith === "success") { + oauthGrantResponse = { + code: "test-oauth-code", + }; + } else if (respondWith === "timeout") { + oauthGrantResponse = "timeout"; + } else { + oauthGrantResponse = respondWith; + } + }; + + const mockRevokeAuthorization = () => { + const outcome = { + actual: new Request("https://example.org"), + expected: new Request("https://dash.cloudflare.com/oauth2/revoke", { + method: "POST", + }), + }; + + fetchMock.mockIf(outcome.expected.url, async (req) => { + outcome.actual = req; + return ""; + }); + + return outcome; + }; + + const mockGrantAccessToken = ({ + respondWith, + }: { + respondWith: MockTokenResponse; + }) => { + const outcome = { + actual: new Request("https://example.org"), + expected: new Request("https://dash.cloudflare.com/oauth2/token", { + method: "POST", + }), + }; + + fetchMock.mockOnceIf(outcome.expected.url, async (req) => { + outcome.actual = req; + return makeTokenResponse(respondWith); + }); + + return outcome; + }; + + return { + mockGrantAuthorization, + mockRevokeAuthorization, + mockGrantAccessToken, + }; +}; + +type GrantResponseOptions = { + code?: string; + error?: ErrorType | ErrorType[]; +}; + +const toQueryParams = ( + { code, error }: GrantResponseOptions, + wranglerRequestParams: URLSearchParams +): string => { + const queryParams = []; + if (code) { + queryParams.push(`code=${code}`); + } + if (error) { + const stringifiedErr = Array.isArray(error) ? error.join(",") : error; + queryParams.push(`error=${stringifiedErr}`); + } + + queryParams.push(`state=${wranglerRequestParams.get("state")}`); + + return queryParams.join("&"); +}; + +type ErrorType = + | "invalid_request" + | "invalid_grant" + | "unauthorized_client" + | "access_denied" + | "unsupported_response_type" + | "invalid_scope" + | "server_error" + | "temporarily_unavailable" + | "invalid_client" + | "unsupported_grant_type" + | "invalid_json" + | "invalid_token" + | "test-token-grant-error"; + +type MockTokenResponse = + | "ok" + | "error" + | { + access_token: string; + expires_in: number; + refresh_token: string; + scope: string; + } + | { + error: ErrorType; + }; + +const makeTokenResponse = (partialResponse: MockTokenResponse): string => { + let fullResponse: MockTokenResponse; + + if (partialResponse === "ok") { + fullResponse = { + access_token: "test-access-token", + expires_in: 100000, + refresh_token: "test-refresh-token", + scope: "account:read", + }; + } else if (partialResponse === "error") { + fullResponse = { + error: "test-token-grant-error", + }; + } else { + fullResponse = partialResponse; + } + + return JSON.stringify(fullResponse); +}; diff --git a/packages/wrangler/src/__tests__/jest.setup.ts b/packages/wrangler/src/__tests__/jest.setup.ts index f2640df40bff..5bffe0b20a9c 100644 --- a/packages/wrangler/src/__tests__/jest.setup.ts +++ b/packages/wrangler/src/__tests__/jest.setup.ts @@ -5,7 +5,9 @@ import { getCloudflareAPIBaseURL, } from "../cfetch/internal"; import { confirm, prompt } from "../dialogs"; +import openInBrowser from "../open-in-browser"; import { mockFetchInternal, mockFetchKVGetValue } from "./helpers/mock-cfetch"; +import { mockOpenInBrowser as mockOpenInBrowserForOAuthFlow } from "./helpers/mock-oauth-flow"; import { MockWebSocket } from "./helpers/mock-web-socket"; jest.mock("ws", () => { @@ -67,3 +69,6 @@ jest.mock("../dev/dev", () => { return null; }); }); + +jest.mock("../open-in-browser"); +(openInBrowser as jest.Mock).mockImplementation(mockOpenInBrowserForOAuthFlow); diff --git a/packages/wrangler/src/__tests__/logout.test.ts b/packages/wrangler/src/__tests__/logout.test.ts deleted file mode 100644 index 2824934c198f..000000000000 --- a/packages/wrangler/src/__tests__/logout.test.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { existsSync } from "node:fs"; -import os from "node:os"; -import path from "node:path"; -import fetchMock from "jest-fetch-mock"; -import { - reinitialiseAuthTokens, - USER_AUTH_CONFIG_FILE, - writeAuthConfigFile, -} from "../user"; -import { mockConsoleMethods } from "./helpers/mock-console"; -import { runInTempDir } from "./helpers/run-in-tmp"; -import { runWrangler } from "./helpers/run-wrangler"; - -describe("wrangler", () => { - runInTempDir({ homedir: "./home" }); - const std = mockConsoleMethods(); - - describe("logout", () => { - it("should exit with a message stating the user is not logged in", async () => { - await runWrangler("logout"); - expect(std.out).toMatchInlineSnapshot(`"Not logged in, exiting..."`); - }); - - it("should logout user that has been properly logged in", async () => { - writeAuthConfigFile({ - oauth_token: "some-oauth-tok", - refresh_token: "some-refresh-tok", - }); - // Mock out the response for a request to revoke the auth tokens, - // checking the form of the request is as expected. - fetchMock.mockResponseOnce(async (req) => { - expect(req.url).toEqual("https://dash.cloudflare.com/oauth2/revoke"); - expect(req.method).toEqual("POST"); - return ""; - }); - - // Now that we have changed the auth tokens in the wrangler.toml we must reinitialize the user auth state. - reinitialiseAuthTokens(); - await runWrangler("logout"); - - expect(std.out).toMatchInlineSnapshot(`"Successfully logged out."`); - - // Make sure that we made the request to logout. - expect(fetchMock).toHaveBeenCalledTimes(1); - - // Make sure that logout removed the config file containing the auth tokens. - expect(existsSync(path.join(os.homedir(), USER_AUTH_CONFIG_FILE))).toBe( - false - ); - }); - }); -}); diff --git a/packages/wrangler/src/__tests__/publish.test.ts b/packages/wrangler/src/__tests__/publish.test.ts index 66bc263440b3..56447530737c 100644 --- a/packages/wrangler/src/__tests__/publish.test.ts +++ b/packages/wrangler/src/__tests__/publish.test.ts @@ -1,6 +1,7 @@ import * as fs from "node:fs"; import * as path from "node:path"; import * as TOML from "@iarna/toml"; +import { writeAuthConfigFile } from "../user"; import { mockAccountId, mockApiToken } from "./helpers/mock-account-id"; import { createFetchResult, @@ -11,6 +12,7 @@ import { } from "./helpers/mock-cfetch"; import { mockConsoleMethods } from "./helpers/mock-console"; import { mockKeyListRequest } from "./helpers/mock-kv"; +import { mockOAuthFlow } from "./helpers/mock-oauth-flow"; import { runInTempDir } from "./helpers/run-in-tmp"; import { runWrangler } from "./helpers/run-wrangler"; import writeWranglerToml from "./helpers/write-wrangler-toml"; @@ -19,22 +21,93 @@ import type { KVNamespaceInfo } from "../kv"; import type { FormData, File } from "undici"; describe("publish", () => { + mockAccountId(); + mockApiToken(); + runInTempDir({ homedir: "./home" }); + const std = mockConsoleMethods(); + const { mockGrantAccessToken, mockGrantAuthorization } = mockOAuthFlow(); + beforeEach(() => { // @ts-expect-error we're using a very simple setTimeout mock here jest.spyOn(global, "setTimeout").mockImplementation((fn, _period) => { - fn(); + setImmediate(fn); }); }); - mockAccountId(); - mockApiToken(); - runInTempDir(); - const std = mockConsoleMethods(); afterEach(() => { unsetAllMocks(); unsetMockFetchKVGetValues(); }); + describe("authentication", () => { + mockApiToken({ apiToken: null }); + beforeEach(() => { + // @ts-expect-error disable the mock we'd setup earlier + // or else our server won't bother listening for oauth requests + // and will timeout and fail + global.setTimeout.mockRestore(); + }); + + it("drops a user into the login flow if they're unauthenticated", async () => { + writeWranglerToml(); + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + + const accessTokenRequest = mockGrantAccessToken({ respondWith: "ok" }); + mockGrantAuthorization({ respondWith: "success" }); + + await expect(runWrangler("publish index.js")).resolves.toBeUndefined(); + + expect(accessTokenRequest.actual.url).toEqual( + accessTokenRequest.expected.url + ); + + expect(std.out).toMatchInlineSnapshot(` + "Attempting to login via OAuth... + Successfully logged in. + Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + test-name.test-sub-domain.workers.dev" + `); + expect(std.warn).toMatchInlineSnapshot(`""`); + expect(std.err).toMatchInlineSnapshot(`""`); + }); + + it("warns a user when they're authenticated with an API token in wrangler config file", async () => { + writeWranglerToml(); + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + writeAuthConfigFile({ + api_token: "some-api-token", + }); + + const accessTokenRequest = mockGrantAccessToken({ respondWith: "ok" }); + mockGrantAuthorization({ respondWith: "success" }); + + await expect(runWrangler("publish index.js")).resolves.toBeUndefined(); + + expect(accessTokenRequest.actual.url).toEqual( + accessTokenRequest.expected.url + ); + + expect(std.out).toMatchInlineSnapshot(` + "Attempting to login via OAuth... + Successfully logged in. + Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + test-name.test-sub-domain.workers.dev" + `); + expect(std.warn).toMatchInlineSnapshot(` + "It looks like you have used Wrangler 1's \`config\` command to login with an API token. + This is no longer supported in the current version of Wrangler. + If you wish to authenticate via an API token then please set the \`CLOUDFLARE_API_TOKEN\` environment variable." + `); + expect(std.err).toMatchInlineSnapshot(`""`); + }); + }); + describe("environments", () => { it("should use legacy environments by default", async () => { writeWranglerToml({ env: { "some-env": {} } }); diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts new file mode 100644 index 000000000000..d5ea8c0028c9 --- /dev/null +++ b/packages/wrangler/src/__tests__/user.test.ts @@ -0,0 +1,83 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import fetchMock from "jest-fetch-mock"; +import { + readAuthConfigFile, + USER_AUTH_CONFIG_FILE, + writeAuthConfigFile, +} from "../user"; +import { mockConsoleMethods } from "./helpers/mock-console"; +import { mockOAuthFlow } from "./helpers/mock-oauth-flow"; +import { runInTempDir } from "./helpers/run-in-tmp"; +import { runWrangler } from "./helpers/run-wrangler"; +import type { UserAuthConfig } from "../user"; + +describe("wrangler", () => { + runInTempDir({ homedir: "./home" }); + const std = mockConsoleMethods(); + const { + mockGrantAccessToken, + mockGrantAuthorization, + mockRevokeAuthorization, + } = mockOAuthFlow(); + + describe("login", () => { + it("should should log in a user when `wrangler login` is run", async () => { + const accessTokenRequest = mockGrantAccessToken({ respondWith: "ok" }); + mockGrantAuthorization({ respondWith: "success" }); + + await runWrangler("login"); + + expect(accessTokenRequest.actual.url).toEqual( + accessTokenRequest.expected.url + ); + expect(accessTokenRequest.actual.method).toEqual( + accessTokenRequest.expected.method + ); + + expect(std.out).toMatchInlineSnapshot(` + "Attempting to login via OAuth... + Successfully logged in." + `); + + expect(readAuthConfigFile()).toEqual({ + api_token: undefined, + oauth_token: "test-access-token", + refresh_token: "test-refresh-token", + expiration_time: expect.any(String), + }); + }); + }); + + describe("logout", () => { + it("should exit with a message stating the user is not logged in", async () => { + await runWrangler("logout"); + expect(std.out).toMatchInlineSnapshot(`"Not logged in, exiting..."`); + }); + + it("should logout user that has been properly logged in", async () => { + writeAuthConfigFile({ + oauth_token: "some-oauth-tok", + refresh_token: "some-refresh-tok", + }); + const outcome = mockRevokeAuthorization(); + + await runWrangler("logout"); + + expect(outcome.actual.url).toEqual( + "https://dash.cloudflare.com/oauth2/revoke" + ); + expect(outcome.actual.method).toEqual("POST"); + + expect(std.out).toMatchInlineSnapshot(`"Successfully logged out."`); + + // Make sure that we made the request to logout. + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Make sure that logout removed the config file containing the auth tokens. + const config = path.join(os.homedir(), USER_AUTH_CONFIG_FILE); + expect(fs.existsSync(config)).toBeFalsy(); + }); + }); +}); diff --git a/packages/wrangler/src/__tests__/whoami.test.tsx b/packages/wrangler/src/__tests__/whoami.test.tsx index d273a8b68b7b..6c6e613313e1 100644 --- a/packages/wrangler/src/__tests__/whoami.test.tsx +++ b/packages/wrangler/src/__tests__/whoami.test.tsx @@ -1,6 +1,6 @@ import { render } from "ink-testing-library"; import React from "react"; -import { reinitialiseAuthTokens, writeAuthConfigFile } from "../user"; +import { writeAuthConfigFile } from "../user"; import { getUserInfo, WhoAmI } from "../whoami"; import { setMockResponse } from "./helpers/mock-cfetch"; import { mockConsoleMethods } from "./helpers/mock-console"; @@ -18,16 +18,12 @@ describe("getUserInfo()", () => { it("should return undefined if there is an empty config file", async () => { writeAuthConfigFile({}); - // Now that we have changed the auth tokens in the wrangler.toml we must reinitialize the user auth state. - reinitialiseAuthTokens(); const userInfo = await getUserInfo(); expect(userInfo).toBeUndefined(); }); it("should return the user's email and accounts if authenticated via config token", async () => { writeAuthConfigFile({ oauth_token: "some-oauth-token" }); - // Now that we have changed the auth tokens in the wrangler.toml we must reinitialize the user auth state. - reinitialiseAuthTokens(); setMockResponse("/user", () => { return { email: "user@example.com" }; @@ -56,7 +52,6 @@ describe("getUserInfo()", () => { it("should display a warning message if the config file contains a legacy api_token field", async () => { writeAuthConfigFile({ api_token: "API_TOKEN" }); - await reinitialiseAuthTokens(); await getUserInfo(); expect(std.warn).toMatchInlineSnapshot(` "It looks like you have used Wrangler 1's \`config\` command to login with an API token. diff --git a/packages/wrangler/src/user.tsx b/packages/wrangler/src/user.tsx index 5f579cb55162..ee4d94813e58 100644 --- a/packages/wrangler/src/user.tsx +++ b/packages/wrangler/src/user.tsx @@ -345,7 +345,7 @@ let LocalState: State = getAuthTokens(); /** * Compute the current auth tokens. */ -function getAuthTokens(): AuthTokens { +function getAuthTokens(config?: UserAuthConfig): AuthTokens { // get refreshToken/accessToken from fs if exists try { // if the environment variable is available, use that @@ -361,7 +361,7 @@ function getAuthTokens(): AuthTokens { // otherwise try loading from the user auth config file. const { oauth_token, refresh_token, expiration_time, api_token } = - readAuthConfigFile(); + config || readAuthConfigFile(); if (oauth_token) { return { @@ -384,9 +384,20 @@ function getAuthTokens(): AuthTokens { /** * Run the initialisation of the auth state, in the case that something changed. + * + * This runs automatically whenever `writeAuthConfigFile` is run, so generally + * you won't need to call it yourself. + */ +export function reinitialiseAuthTokens(): void; + +/** + * Reinitialise auth state from an in-memory config, skipping + * over the part where we write a file and then read it back into memory */ -export function reinitialiseAuthTokens(): void { - LocalState = getAuthTokens(); +export function reinitialiseAuthTokens(config: UserAuthConfig): void; + +export function reinitialiseAuthTokens(config?: UserAuthConfig): void { + LocalState = getAuthTokens(config); } export function getAPIToken(): string | undefined { @@ -832,6 +843,10 @@ function generateRandomState(lengthOfState: number): string { .join(""); } +/** + * Writes a a wrangler config file (auth credentials) to disk, + * and updates the user auth state with the new credentials. + */ export function writeAuthConfigFile(config: UserAuthConfig) { mkdirSync(path.join(os.homedir(), ".wrangler/config/"), { recursive: true, @@ -841,9 +856,11 @@ export function writeAuthConfigFile(config: UserAuthConfig) { TOML.stringify(config as TOML.JsonMap), { encoding: "utf-8" } ); + + reinitialiseAuthTokens(); } -function readAuthConfigFile(): UserAuthConfig { +export function readAuthConfigFile(): UserAuthConfig { const toml = TOML.parse( readFileSync(path.join(os.homedir(), USER_AUTH_CONFIG_FILE)) ); @@ -882,7 +899,6 @@ export async function loginOrRefreshIfRequired( export async function login(props?: LoginProps): Promise { console.log("Attempting to login via OAuth..."); const urlToOpen = await getAuthURL(props?.scopes); - await openInBrowser(urlToOpen); let server: http.Server; let loginTimeoutHandle: NodeJS.Timeout; const timerPromise = new Promise((resolve) => { @@ -963,6 +979,8 @@ export async function login(props?: LoginProps): Promise { server.listen(8976); }); + await openInBrowser(urlToOpen); + return Promise.race([timerPromise, loginPromise]); }