Skip to content

Commit

Permalink
non-TTY check for required variables:
Browse files Browse the repository at this point in the history
Added a check in non-TTY environments for `account_id`, `CLOUDFLARE_ACCOUNT_ID` and `CLOUDFLARE_API_TOKEN`. If `account_id` exists in `wrangler.toml`
then `CLOUDFLARE_ACCOUNT_ID` is not needed in non-TTY scope. The `CLOUDFLARE_API_TOKEN` is necessary in non-TTY scope and will always error if missing.

resolves #827
  • Loading branch information
JacobMGEvans authored and petebacondarwin committed May 3, 2022
1 parent 97f945f commit cd17369
Show file tree
Hide file tree
Showing 10 changed files with 232 additions and 49 deletions.
9 changes: 9 additions & 0 deletions .changeset/eighty-yaks-jump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"wrangler": patch
---

feat: non-TTY check for required variables
Added a check in non-TTY environments for `account_id`, `CLOUDFLARE_ACCOUNT_ID` and `CLOUDFLARE_API_TOKEN`. If `account_id` exists in `wrangler.toml`
then `CLOUDFLARE_ACCOUNT_ID` is not needed in non-TTY scope. The `CLOUDFLARE_API_TOKEN` is necessary in non-TTY scope and will always error if missing.

resolves #827
10 changes: 7 additions & 3 deletions packages/wrangler/src/__tests__/helpers/mock-istty.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const ORIGINAL_ISTTY = process.stdout.isTTY;
const ORIGINAL_STDOUT_ISTTY = process.stdout.isTTY;
const ORIGINAL_STDIN_ISTTY = process.stdin.isTTY;

/**
* Mock `process.stdout.isTTY`
Expand All @@ -9,14 +10,17 @@ export function useMockIsTTY() {
*/
const setIsTTY = (isTTY: boolean) => {
process.stdout.isTTY = isTTY;
process.stdin.isTTY = isTTY;
};

beforeEach(() => {
process.stdout.isTTY = ORIGINAL_ISTTY;
process.stdout.isTTY = ORIGINAL_STDOUT_ISTTY;
process.stdin.isTTY = ORIGINAL_STDIN_ISTTY;
});

afterEach(() => {
process.stdout.isTTY = ORIGINAL_ISTTY;
process.stdout.isTTY = ORIGINAL_STDOUT_ISTTY;
process.stdin.isTTY = ORIGINAL_STDIN_ISTTY;
});

return { setIsTTY };
Expand Down
28 changes: 26 additions & 2 deletions packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fetchMock from "jest-fetch-mock";
import { Request } from "undici";
import { getCloudflareApiBaseUrl } from "../../cfetch";
import openInBrowser from "../../open-in-browser";
import { mockHttpServer } from "./mock-http-server";

Expand Down Expand Up @@ -85,6 +86,28 @@ export const mockOAuthFlow = () => {
return outcome;
};

const mockGetMemberships = (args: {
success: boolean;
result: { id: string; account: { id: string; name: string } }[];
}) => {
const outcome = {
actual: new Request("https://example.org"),
expected: new Request(`${getCloudflareApiBaseUrl()}/memberships`, {
method: "GET",
}),
};

fetchMock.mockIf(outcome.expected.url, async (req) => {
outcome.actual = req;
return {
status: 200,
body: JSON.stringify(args),
};
});

return outcome;
};

const mockGrantAccessToken = ({
respondWith,
}: {
Expand Down Expand Up @@ -150,10 +173,11 @@ export const mockOAuthFlow = () => {
};

return {
mockOAuthServerCallback,
mockGetMemberships,
mockGrantAccessToken,
mockGrantAuthorization,
mockOAuthServerCallback,
mockRevokeAuthorization,
mockGrantAccessToken,
mockExchangeRefreshTokenForAccessToken,
};
};
Expand Down
126 changes: 126 additions & 0 deletions packages/wrangler/src/__tests__/publish.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
unsetMockFetchKVGetValues,
} from "./helpers/mock-cfetch";
import { mockConsoleMethods } from "./helpers/mock-console";
import { useMockIsTTY } from "./helpers/mock-istty";
import { mockKeyListRequest } from "./helpers/mock-kv";
import { mockOAuthFlow } from "./helpers/mock-oauth-flow";
import { runInTempDir } from "./helpers/run-in-tmp";
Expand All @@ -27,18 +28,21 @@ describe("publish", () => {
mockAccountId();
mockApiToken();
runInTempDir({ homedir: "./home" });
const { setIsTTY } = useMockIsTTY();
const std = mockConsoleMethods();
const {
mockOAuthServerCallback,
mockGrantAccessToken,
mockGrantAuthorization,
mockGetMemberships,
} = mockOAuthFlow();

beforeEach(() => {
// @ts-expect-error we're using a very simple setTimeout mock here
jest.spyOn(global, "setTimeout").mockImplementation((fn, _period) => {
setImmediate(fn);
});
setIsTTY(true);
});

afterEach(() => {
Expand All @@ -56,6 +60,7 @@ describe("publish", () => {
});

it("drops a user into the login flow if they're unauthenticated", async () => {
// Should not throw missing Errors in TTY environment
writeWranglerToml();
writeWorkerSource();
mockSubDomainRequest();
Expand Down Expand Up @@ -116,6 +121,127 @@ describe("publish", () => {
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});

describe("non-TTY", () => {
const ENV_COPY = process.env;

afterEach(() => {
process.env = ENV_COPY;
});

it("should not throw an error in non-TTY if 'CLOUDFLARE_API_TOKEN' & 'account_id' are in scope", async () => {
process.env = {
CLOUDFLARE_API_TOKEN: "123456789",
};
setIsTTY(false);
writeWranglerToml({
account_id: "some-account-id",
});
writeWorkerSource();
mockSubDomainRequest();
mockUploadWorkerRequest();
mockOAuthServerCallback();

await runWrangler("publish index.js");

expect(std.out).toMatchInlineSnapshot(`
"Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should not throw an error if 'CLOUDFLARE_ACCOUNT_ID' & 'CLOUDFLARE_API_TOKEN' are in scope", async () => {
process.env = {
CLOUDFLARE_API_TOKEN: "hunter2",
CLOUDFLARE_ACCOUNT_ID: "some-account-id",
};
setIsTTY(false);
writeWranglerToml();
writeWorkerSource();
mockSubDomainRequest();
mockUploadWorkerRequest();
mockOAuthServerCallback();
mockGetMemberships({
success: true,
result: [],
});

await runWrangler("publish index.js");

expect(std.out).toMatchInlineSnapshot(`
"Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should throw an error in non-TTY if 'account_id' & 'CLOUDFLARE_ACCOUNT_ID' is missing", async () => {
setIsTTY(false);
process.env = {
CLOUDFLARE_API_TOKEN: "hunter2",
CLOUDFLARE_ACCOUNT_ID: undefined,
};
writeWranglerToml({
account_id: undefined,
});
writeWorkerSource();
mockSubDomainRequest();
mockUploadWorkerRequest();
mockOAuthServerCallback();
mockGetMemberships({
success: true,
result: [
{ id: "IG-88", account: { id: "1701", name: "enterprise" } },
{ id: "R2-D2", account: { id: "nx01", name: "enterprise-nx" } },
],
});

await expect(runWrangler("publish index.js")).rejects
.toMatchInlineSnapshot(`
[Error: More than one account available but unable to select one in non-interactive mode.
Please set the appropriate \`account_id\` in your \`wrangler.toml\` file.
Available accounts are ("<name>" - "<id>"):
"enterprise" - "1701")
"enterprise-nx" - "nx01")]
`);
});

it("should throw error in non-TTY if 'CLOUDFLARE_API_TOKEN' is missing", async () => {
setIsTTY(false);
writeWranglerToml({
account_id: undefined,
});
process.env = {
CLOUDFLARE_API_TOKEN: undefined,
CLOUDFLARE_ACCOUNT_ID: "badwolf",
};
writeWorkerSource();
mockSubDomainRequest();
mockUploadWorkerRequest();
mockOAuthServerCallback();
mockGetMemberships({
success: true,
result: [
{ id: "IG-88", account: { id: "1701", name: "enterprise" } },
{ id: "R2-D2", account: { id: "nx01", name: "enterprise-nx" } },
],
});

await expect(runWrangler("publish index.js")).rejects.toThrowError();

expect(std.err).toMatchInlineSnapshot(`
"X [ERROR] Missing 'CLOUDFLARE_API_TOKEN' from non-TTY environment, please see docs for more info: TBD
X [ERROR] Did not login, quitting...
"
`);
});
});
});

describe("environments", () => {
Expand Down
49 changes: 27 additions & 22 deletions packages/wrangler/src/__tests__/secret.test.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
import * as fs from "node:fs";
import * as TOML from "@iarna/toml";
import fetchMock from "jest-fetch-mock";
import { mockAccountId, mockApiToken } from "./helpers/mock-account-id";
import { setMockResponse, unsetAllMocks } from "./helpers/mock-cfetch";
import { mockConsoleMethods } from "./helpers/mock-console";
import { mockConfirm, mockPrompt } from "./helpers/mock-dialogs";
import { useMockIsTTY } from "./helpers/mock-istty";
import { mockOAuthFlow } from "./helpers/mock-oauth-flow";
import { useMockStdin } from "./helpers/mock-stdin";
import { runInTempDir } from "./helpers/run-in-tmp";
import { runWrangler } from "./helpers/run-wrangler";

describe("wrangler secret", () => {
const std = mockConsoleMethods();
const { mockGetMemberships } = mockOAuthFlow();
const { setIsTTY } = useMockIsTTY();
runInTempDir();
mockAccountId();
mockApiToken();

afterEach(() => {
unsetAllMocks();
setIsTTY(true);
});

describe("put", () => {
Expand Down Expand Up @@ -169,6 +173,10 @@ describe("wrangler secret", () => {
mockAccountId({ accountId: null });

it("should error if a user has no account", async () => {
mockGetMemberships({
success: false,
result: [],
});
await expect(
runWrangler("secret put the-key --name script-name")
).rejects.toThrowErrorMatchingInlineSnapshot(
Expand Down Expand Up @@ -196,28 +204,25 @@ describe("wrangler secret", () => {
});

it("should error if a user has multiple accounts, and has not specified an account in wrangler.toml", async () => {
// This is a mock response for the request to the CF API memberships of the current user.
fetchMock.doMockOnce(async () => {
return {
body: JSON.stringify({
success: true,
result: [
{
id: "1",
account: { id: "account-id-1", name: "account-name-1" },
},
{
id: "2",
account: { id: "account-id-2", name: "account-name-2" },
},
{
id: "3",
account: { id: "account-id-3", name: "account-name-3" },
},
],
}),
};
setIsTTY(false);
mockGetMemberships({
success: true,
result: [
{
id: "1",
account: { id: "account-id-1", name: "account-name-1" },
},
{
id: "2",
account: { id: "account-id-2", name: "account-name-2" },
},
{
id: "3",
account: { id: "account-id-3", name: "account-name-3" },
},
],
});

await expect(runWrangler("secret put the-key --name script-name"))
.rejects.toThrowErrorMatchingInlineSnapshot(`
"More than one account available but unable to select one in non-interactive mode.
Expand Down
20 changes: 8 additions & 12 deletions packages/wrangler/src/__tests__/sentry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,27 @@ import * as Sentry from "@sentry/node";
import prompts from "prompts";

import { mockConsoleMethods } from "./helpers/mock-console";
import { useMockIsTTY } from "./helpers/mock-istty";
import { runInTempDir } from "./helpers/run-in-tmp";
import { runWrangler } from "./helpers/run-wrangler";
const { reportError } = jest.requireActual("../reporting");

describe("Error Reporting", () => {
const { setIsTTY } = useMockIsTTY();

runInTempDir({ homedir: "./home" });
mockConsoleMethods();
const reportingTOMLPath = ".wrangler/config/reporting.toml";

const originalTTY = process.stdout.isTTY;
beforeEach(() => {
jest.mock("@sentry/node");
jest.spyOn(Sentry, "captureException");
process.stdout.isTTY = true;
setIsTTY(true);
});

afterEach(() => {
jest.unmock("@sentry/node");
jest.clearAllMocks();
process.stdout.isTTY = originalTTY;
});

it("should confirm user will allow error reporting usage", async () => {
Expand Down Expand Up @@ -127,20 +128,15 @@ describe("Error Reporting", () => {
});

it("should not prompt in non-TTY environment", async () => {
process.stdout.isTTY = false;

setIsTTY(false);
await reportError(new Error("test error"), "testFalse");

const { error_tracking_opt, error_tracking_opt_date } = TOML.parse(
await fsp.readFile(path.join(os.homedir(), reportingTOMLPath), "utf-8")
);

expect(error_tracking_opt).toBe(false);
expect(error_tracking_opt_date).toBeTruthy();
expect(
fs.existsSync(path.join(os.homedir(), reportingTOMLPath, "utf-8"))
).toBe(false);

expect(Sentry.captureException).not.toHaveBeenCalledWith(
new Error("test error")
);
process.stdout.isTTY = originalTTY;
});
});
Loading

0 comments on commit cd17369

Please sign in to comment.