Improve login error handling #794

JacobMGEvans · 2022-04-13T17:43:13Z

Error messaging from failed login would dump a JSON.parse error in some situations. Added a fallback if .json fails to parse it will attempt .text() then throw result. If both attempts to parse fail it will throw an UnknownError with a message showing where it originated.
resolves #539

changeset-bot · 2022-04-13T17:43:17Z

🦋 Changeset detected

Latest commit: 0dd05c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

github-actions · 2022-04-13T17:44:08Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2196591553/npm-package-wrangler-794

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/794/npm-package-wrangler-794

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2196591553/npm-package-wrangler-794 dev path/to/script.js

packages/wrangler/src/user.tsx

threepointone · 2022-04-14T13:07:56Z

This probably deserves a test.

JacobMGEvans · 2022-04-14T13:48:46Z

This probably deserves a test.

That's fair 😅

JacobMGEvans · 2022-04-15T23:31:59Z

At a lost on how to test this particular function. Will come back to it.

threepointone · 2022-04-15T23:48:03Z

packages/wrangler/src/__tests__/user.test.ts

@@ -48,6 +51,25 @@ describe("wrangler", () => {
        expiration_time: expect.any(String),
      });
    });
+
+    it.only("should confirm error message and output for failed login", async () => {


forgot to remove the .only here?

threepointone · 2022-04-16T16:07:39Z

packages/wrangler/src/user.tsx

+
+    if (tokenExhangeResErr !== undefined) {
+      // We will throw the parsed error if it parsed correctly, otherwise we throw an unknown error.
+      throw tokenExhangeResErr;


Suggested change

throw tokenExhangeResErr;

throw typeof tokenExhangeResErr === 'string' ? new Error(tokenExhangeResErr): tokenExhangeResErr;

petebacondarwin · 2022-04-18T16:46:48Z

I think you need to make this change to get the test not to hang (and to pass):

diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts
index b043048..3d8edb7 100644
--- a/packages/wrangler/src/__tests__/user.test.ts
+++ b/packages/wrangler/src/__tests__/user.test.ts
@@ -54,23 +54,19 @@ describe("wrangler", () => {
       });
     });
 
-    it.only("should confirm error message and output for failed login", async () => {
-      const accessTokenRequest = mockGrantAccessToken({ respondWith: "ok" });
+    it("should confirm error message and output for failed login", async () => {
+      mockOAuthServerCallback();
+      mockGrantAccessToken({ respondWith: "ok" });
       mockGrantAuthorization({ respondWith: "failure" });
 
       await runWrangler("login");
 
-      expect(accessTokenRequest.actual.url).toEqual("");
-      expect(accessTokenRequest.actual.method).toEqual("");
-
-      expect(std.out).toMatchInlineSnapshot(``);
-
-      expect(readAuthConfigFile()).toEqual<UserAuthConfig>({
-        api_token: undefined,
-        oauth_token: "test-access-token",
-        refresh_token: "test-refresh-token",
-        expiration_time: expect.any(String),
-      });
+      expect(std.out).toMatchInlineSnapshot(`
+        "Attempting to login via OAuth...
+        Error: Consent denied. You must grant consent to Wrangler in order to login.
+        If you don't want to do this consider passing an API token via the \`CLOUDFLARE_API_TOKEN\` environment variable"
+      `);
+      expect(std.warn).toMatchInlineSnapshot(`""`);
     });
   });

packages/wrangler/src/user.tsx

JacobMGEvans · 2022-04-18T17:52:50Z

I think you need to make this change to get the test not to hang (and to pass):

diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts
index b043048..3d8edb7 100644
--- a/packages/wrangler/src/__tests__/user.test.ts
+++ b/packages/wrangler/src/__tests__/user.test.ts
@@ -54,23 +54,19 @@ describe("wrangler", () => {
       });
     });
 
-    it.only("should confirm error message and output for failed login", async () => {
-      const accessTokenRequest = mockGrantAccessToken({ respondWith: "ok" });
+    it("should confirm error message and output for failed login", async () => {
+      mockOAuthServerCallback();
+      mockGrantAccessToken({ respondWith: "ok" });
       mockGrantAuthorization({ respondWith: "failure" });
 
       await runWrangler("login");
 
-      expect(accessTokenRequest.actual.url).toEqual("");
-      expect(accessTokenRequest.actual.method).toEqual("");
-
-      expect(std.out).toMatchInlineSnapshot(``);
-
-      expect(readAuthConfigFile()).toEqual<UserAuthConfig>({
-        api_token: undefined,
-        oauth_token: "test-access-token",
-        refresh_token: "test-refresh-token",
-        expiration_time: expect.any(String),
-      });
+      expect(std.out).toMatchInlineSnapshot(`
+        "Attempting to login via OAuth...
+        Error: Consent denied. You must grant consent to Wrangler in order to login.
+        If you don't want to do this consider passing an API token via the \`CLOUDFLARE_API_TOKEN\` environment variable"
+      `);
+      expect(std.warn).toMatchInlineSnapshot(`""`);
     });
   });

@caass graciously helped me figure out how I caused hanging tests. It was silly workaround I was trying to make didn't play nicely with other pieces, now I am trying to mock the response for the token refresh to access function.

JacobMGEvans · 2022-04-19T21:36:11Z

packages/wrangler/src/__tests__/user.test.ts

+        respondWith: "badResponse",
+      });
+
+      await expect(requireAuth({} as Config, false)).rejects.toThrowError();


Thanks for the tip on how to get this workaround to work @petebacondarwin

petebacondarwin

Thanks for driving this fix through @JacobMGEvans - not a simple part of the code base to test.

packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts

.changeset/early-ducks-travel.md

packages/wrangler/src/__tests__/user.test.ts

packages/wrangler/src/user.tsx

packages/wrangler/src/__tests__/user.test.ts

petebacondarwin · 2022-04-20T06:14:53Z

packages/wrangler/src/__tests__/user.test.ts

+      });
+
+      await expect(requireAuth({} as Config, false)).rejects.toThrowError();
+      expect(std.err).toContain(


Can we use toMatchInlineSnapshot() here?

No, it breaks in different environments, the callstack is slightly different but the overall Error is the same

JacobMGEvans · 2022-04-20T13:51:56Z

packages/wrangler/src/__tests__/user.test.ts

 import type { UserAuthConfig } from "../user";

-describe("wrangler", () => {
+describe("User", () => {


User related tests

petebacondarwin

A couple of NITs left in the tests.

packages/wrangler/src/__tests__/user.test.ts

…r in some situations. Added a fallback if `.json` fails to parse it will attempt `.text()` then throw result. If both attempts to parse fail it will throw an `UnknownError` with a message showing where it originated. should resolve #539

JacobMGEvans added the bug label Apr 13, 2022

JacobMGEvans requested review from threepointone and petebacondarwin as code owners April 13, 2022 17:43

JacobMGEvans changed the title ~~Improve Login Error Handling~~ Improve login error handling Apr 13, 2022

threepointone reviewed Apr 14, 2022

View reviewed changes

packages/wrangler/src/user.tsx Outdated Show resolved Hide resolved

JacobMGEvans force-pushed the error-message-login-fail branch from e15fc15 to 3e62b63 Compare April 15, 2022 23:23

threepointone reviewed Apr 15, 2022

View reviewed changes

threepointone reviewed Apr 16, 2022

View reviewed changes

petebacondarwin reviewed Apr 18, 2022

View reviewed changes

packages/wrangler/src/user.tsx Outdated Show resolved Hide resolved

JacobMGEvans force-pushed the error-message-login-fail branch 3 times, most recently from c7d3051 to ab4ef3b Compare April 19, 2022 21:30

JacobMGEvans commented Apr 19, 2022

View reviewed changes

JacobMGEvans force-pushed the error-message-login-fail branch from ab4ef3b to 4983812 Compare April 19, 2022 21:38

petebacondarwin requested changes Apr 20, 2022

View reviewed changes

JacobMGEvans force-pushed the error-message-login-fail branch 3 times, most recently from 04c5730 to 4af9ee0 Compare April 20, 2022 13:51

JacobMGEvans commented Apr 20, 2022

View reviewed changes

JacobMGEvans requested review from petebacondarwin and threepointone April 20, 2022 13:54

petebacondarwin approved these changes Apr 20, 2022

View reviewed changes

packages/wrangler/src/__tests__/user.test.ts Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/user.test.ts Show resolved Hide resolved

JacobMGEvans force-pushed the error-message-login-fail branch from 4af9ee0 to 0dd05c9 Compare April 20, 2022 15:11

JacobMGEvans requested a review from petebacondarwin April 20, 2022 15:12

JacobMGEvans merged commit ee3475f into main Apr 20, 2022

JacobMGEvans deleted the error-message-login-fail branch April 20, 2022 15:53

github-actions bot mentioned this pull request Apr 20, 2022

Version Packages #817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve login error handling #794

Improve login error handling #794

JacobMGEvans commented Apr 13, 2022 •

edited

Loading

changeset-bot bot commented Apr 13, 2022 •

edited

Loading

github-actions bot commented Apr 13, 2022 •

edited

Loading

threepointone commented Apr 14, 2022

JacobMGEvans commented Apr 14, 2022

JacobMGEvans commented Apr 15, 2022

threepointone Apr 15, 2022

JacobMGEvans Apr 18, 2022

threepointone Apr 16, 2022

petebacondarwin commented Apr 18, 2022

JacobMGEvans commented Apr 18, 2022

JacobMGEvans Apr 19, 2022

petebacondarwin left a comment

petebacondarwin Apr 20, 2022

JacobMGEvans Apr 20, 2022

JacobMGEvans Apr 20, 2022

petebacondarwin left a comment

	throw tokenExhangeResErr;
	throw typeof tokenExhangeResErr === 'string' ? new Error(tokenExhangeResErr): tokenExhangeResErr;

Improve login error handling #794

Improve login error handling #794

Conversation

JacobMGEvans commented Apr 13, 2022 • edited Loading

changeset-bot bot commented Apr 13, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 13, 2022 • edited Loading

threepointone commented Apr 14, 2022

JacobMGEvans commented Apr 14, 2022

JacobMGEvans commented Apr 15, 2022

threepointone Apr 15, 2022

Choose a reason for hiding this comment

JacobMGEvans Apr 18, 2022

Choose a reason for hiding this comment

threepointone Apr 16, 2022

Choose a reason for hiding this comment

petebacondarwin commented Apr 18, 2022

JacobMGEvans commented Apr 18, 2022

JacobMGEvans Apr 19, 2022

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Apr 20, 2022

Choose a reason for hiding this comment

JacobMGEvans Apr 20, 2022

Choose a reason for hiding this comment

JacobMGEvans Apr 20, 2022

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

JacobMGEvans commented Apr 13, 2022 •

edited

Loading

changeset-bot bot commented Apr 13, 2022 •

edited

Loading

github-actions bot commented Apr 13, 2022 •

edited

Loading