From 7fdc631aa3725cac407e28e262d4cfad999249c6 Mon Sep 17 00:00:00 2001 From: Roman Balayan Date: Thu, 15 Oct 2020 21:40:27 +0800 Subject: [PATCH 1/7] Add log option parameter on createAppAuth to allow override of console.warn implementation as described on #176 --- src/hook.ts | 10 +++--- src/index.ts | 10 +++++- src/types.ts | 6 ++++ test/index.test.ts | 88 ++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 99 insertions(+), 15 deletions(-) diff --git a/src/hook.ts b/src/hook.ts index 49163951..a423d9d7 100644 --- a/src/hook.ts +++ b/src/hook.ts @@ -61,8 +61,8 @@ export async function hook( 1000 ); - console.warn(error.message); - console.warn( + state.log.warn(error.message); + state.log.warn( `[@octokit/auth-app] GitHub API time and system time are different by ${diff} seconds. Retrying request with the difference accounted for.` ); @@ -86,6 +86,7 @@ export async function hook( endpoint.headers.authorization = `token ${token}`; return sendRequestWithRetries( + state, request, endpoint as EndpointOptions, createdAt @@ -100,6 +101,7 @@ export async function hook( * @see https://github.com/octokit/auth-app.js/issues/65 */ async function sendRequestWithRetries( + state: State, request: RequestInterface, options: EndpointOptions, createdAt: string, @@ -126,13 +128,13 @@ async function sendRequestWithRetries( ++retries; const awaitTime = retries * 1000; - console.warn( + state.log.warn( `[@octokit/auth-app] Retrying after 401 response to account for token replication delay (retry: ${retries}, wait: ${ awaitTime / 1000 }s)` ); await new Promise((resolve) => setTimeout(resolve, awaitTime)); - return sendRequestWithRetries(request, options, createdAt, retries); + return sendRequestWithRetries(state, request, options, createdAt, retries); } } diff --git a/src/index.ts b/src/index.ts index fa92906c..1fdf6b0f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -37,7 +37,15 @@ export const createAppAuth: StrategyInterface = function createAppAuth( }, options.installationId ? { installationId: Number(options.installationId) } - : {} + : {}, + { + log: Object.assign( + { + warn: console.warn.bind(console), + }, + options.log + ), + } ); return Object.assign(auth.bind(null, state), { diff --git a/src/types.ts b/src/types.ts index 52e241d5..b1933c72 100644 --- a/src/types.ts +++ b/src/types.ts @@ -82,6 +82,9 @@ export type StrategyOptions = { request?: OctokitTypes.RequestInterface; cache?: Cache; timeDifference?: number; + log?: { + warn: (message: string, additionalInfo?: object) => any; + }; }; export type StrategyOptionsWithDefaults = StrategyOptions & { @@ -121,4 +124,7 @@ export type State = StrategyOptions & { installationId?: number; request: OctokitTypes.RequestInterface; cache: Cache; + log: { + warn: (message: string, additionalInfo?: object) => any; + }; }; diff --git a/test/index.test.ts b/test/index.test.ts index ae283de4..17e02fd3 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1243,9 +1243,12 @@ test("auth.hook(): handle 401 due to an exp timestamp in the past", async () => }; }); + global.console.warn = jest.fn(); + const auth = createAppAuth({ id: APP_ID, privateKey: PRIVATE_KEY, + log: global.console, }); const requestWithMock = request.defaults({ @@ -1259,8 +1262,6 @@ test("auth.hook(): handle 401 due to an exp timestamp in the past", async () => }, }); - global.console.warn = jest.fn(); - const promise = requestWithAuth("GET /app"); const { data } = await promise; @@ -1315,9 +1316,12 @@ test("auth.hook(): handle 401 due to an exp timestamp in the past with 800 secon }; }); + global.console.warn = jest.fn(); + const auth = createAppAuth({ id: APP_ID, privateKey: PRIVATE_KEY, + log: global.console, }); const requestWithMock = request.defaults({ @@ -1331,8 +1335,6 @@ test("auth.hook(): handle 401 due to an exp timestamp in the past with 800 secon }, }); - global.console.warn = jest.fn(); - const promise = requestWithAuth("GET /app"); const { data } = await promise; @@ -1385,9 +1387,12 @@ test("auth.hook(): handle 401 due to an iat timestamp in the future", async () = }; }); + global.console.warn = jest.fn(); + const auth = createAppAuth({ id: APP_ID, privateKey: PRIVATE_KEY, + log: global.console, }); const requestWithMock = request.defaults({ @@ -1401,8 +1406,6 @@ test("auth.hook(): handle 401 due to an iat timestamp in the future", async () = }, }); - global.console.warn = jest.fn(); - const promise = requestWithAuth("GET /app"); const { data } = await promise; @@ -1440,9 +1443,12 @@ test("auth.hook(): throw 401 error in app auth flow without timing errors", asyn }, }); + global.console.warn = jest.fn(); + const auth = createAppAuth({ id: APP_ID, privateKey: PRIVATE_KEY, + log: global.console, }); const requestWithMock = request.defaults({ @@ -1456,8 +1462,6 @@ test("auth.hook(): throw 401 error in app auth flow without timing errors", asyn }, }); - global.console.warn = jest.fn(); - try { await requestWithAuth("GET /app"); throw new Error("Should not resolve"); @@ -1518,10 +1522,13 @@ test("auth.hook(): handle 401 in first 5 seconds (#65)", async () => { } ); + global.console.warn = jest.fn(); + const auth = createAppAuth({ id: APP_ID, privateKey: PRIVATE_KEY, installationId: 123, + log: global.console, }); const requestWithMock = request.defaults({ @@ -1538,8 +1545,6 @@ test("auth.hook(): handle 401 in first 5 seconds (#65)", async () => { }, }); - global.console.warn = jest.fn(); - const promise = requestWithAuth("GET /repos/octocat/hello-world"); // it takes 3 retries until a total time of more than 5s pass @@ -1831,3 +1836,66 @@ test("id and installationId can be passed as options", async () => { expect(authentication.token).toEqual("secret123"); }); + +test("createAppAuth passed with log option", async () => { + const calls: String[] = []; + + const mock = fetchMock + .sandbox() + .get("https://api.github.com/app", (_url, options) => { + const auth = (options.headers as { [key: string]: string | undefined })[ + "authorization" + ]; + const [_, jwt] = (auth || "").split(" "); + const payload = JSON.parse(atob(jwt.split(".")[1])); + if (payload.exp < 600) { + return { + status: 401, + body: { + message: + "'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires.", + documentation_url: "https://developer.github.com/v3", + }, + headers: { + date: new Date(Date.now() + 30000).toUTCString(), + }, + }; + } + + return { + status: 200, + body: [], + }; + }); + + const auth = createAppAuth({ + id: APP_ID, + privateKey: PRIVATE_KEY, + log: { + debug: () => calls.push("debug"), + info: () => calls.push("info"), + warn: () => calls.push("warn"), + error: () => calls.push("error"), + }, + }); + + const requestWithMock = request.defaults({ + request: { + fetch: mock, + }, + }); + const requestWithAuth = requestWithMock.defaults({ + request: { + hook: auth.hook, + }, + }); + + const promise = requestWithAuth("GET /app"); + + const { data } = await promise; + + expect(data).toStrictEqual([]); + expect(mock.done()).toBe(true); + + expect(calls).toStrictEqual(["warn", "warn"]); +}); From 30d39d73fc81503383142d312a2dee0222907654 Mon Sep 17 00:00:00 2001 From: Roman Balayan Date: Thu, 15 Oct 2020 21:55:06 +0800 Subject: [PATCH 2/7] Update readme to include log option --- README.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/README.md b/README.md index 644bc4fb..a443244f 100644 --- a/README.md +++ b/README.md @@ -229,6 +229,25 @@ createAppAuth({ }); ``` + + + + log + + + object + + + You can pass in your preferred logging tool by passing option.log to the constructor. If you would like to make the log level configurable using an environment variable or external option, we recommend the console-log-level package. For example: + +```js +createAppAuth({ + clientId: 123, + clientSecret: "secret", + log: require("console-log-level")({ level: "info" }), +}); +``` + From 9a9cb8ef273955178862cab03b4563810e59239a Mon Sep 17 00:00:00 2001 From: Roman Balayan Date: Thu, 15 Oct 2020 22:02:49 +0800 Subject: [PATCH 3/7] WIP: added override for debug, info, and error log methods --- src/index.ts | 3 +++ src/types.ts | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/src/index.ts b/src/index.ts index 1fdf6b0f..97705766 100644 --- a/src/index.ts +++ b/src/index.ts @@ -41,7 +41,10 @@ export const createAppAuth: StrategyInterface = function createAppAuth( { log: Object.assign( { + debug: () => {}, + info: () => {}, warn: console.warn.bind(console), + error: () => {}, }, options.log ), diff --git a/src/types.ts b/src/types.ts index b1933c72..698b9984 100644 --- a/src/types.ts +++ b/src/types.ts @@ -83,7 +83,11 @@ export type StrategyOptions = { cache?: Cache; timeDifference?: number; log?: { + debug: (message: string, additionalInfo?: object) => any; + info: (message: string, additionalInfo?: object) => any; warn: (message: string, additionalInfo?: object) => any; + error: (message: string, additionalInfo?: object) => any; + [key: string]: any; }; }; @@ -125,6 +129,10 @@ export type State = StrategyOptions & { request: OctokitTypes.RequestInterface; cache: Cache; log: { + debug: (message: string, additionalInfo?: object) => any; + info: (message: string, additionalInfo?: object) => any; warn: (message: string, additionalInfo?: object) => any; + error: (message: string, additionalInfo?: object) => any; + [key: string]: any; }; }; From 5b84545662c4c2728c7599d59609343e77b531ae Mon Sep 17 00:00:00 2001 From: Gregor Martynus Date: Thu, 15 Oct 2020 14:11:09 -0700 Subject: [PATCH 4/7] Update src/index.ts --- src/index.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 97705766..1fdf6b0f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -41,10 +41,7 @@ export const createAppAuth: StrategyInterface = function createAppAuth( { log: Object.assign( { - debug: () => {}, - info: () => {}, warn: console.warn.bind(console), - error: () => {}, }, options.log ), From b0593024a761d21606488c63ad47e2dcb7077281 Mon Sep 17 00:00:00 2001 From: Gregor Martynus Date: Thu, 15 Oct 2020 14:11:17 -0700 Subject: [PATCH 5/7] Update test/index.test.ts --- test/index.test.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/index.test.ts b/test/index.test.ts index 17e02fd3..083fd06d 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1872,10 +1872,7 @@ test("createAppAuth passed with log option", async () => { id: APP_ID, privateKey: PRIVATE_KEY, log: { - debug: () => calls.push("debug"), - info: () => calls.push("info"), warn: () => calls.push("warn"), - error: () => calls.push("error"), }, }); From 5cf6bbd371529a32b0f8206b0fc100332939842c Mon Sep 17 00:00:00 2001 From: Gregor Martynus Date: Thu, 15 Oct 2020 14:12:49 -0700 Subject: [PATCH 6/7] Update src/types.ts --- src/types.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/types.ts b/src/types.ts index 698b9984..2119c6a2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -83,10 +83,7 @@ export type StrategyOptions = { cache?: Cache; timeDifference?: number; log?: { - debug: (message: string, additionalInfo?: object) => any; - info: (message: string, additionalInfo?: object) => any; warn: (message: string, additionalInfo?: object) => any; - error: (message: string, additionalInfo?: object) => any; [key: string]: any; }; }; From e5d63c1f8047c9c133ffd5f4a78b56be0fa3bf59 Mon Sep 17 00:00:00 2001 From: Gregor Martynus Date: Thu, 15 Oct 2020 14:13:16 -0700 Subject: [PATCH 7/7] Update src/types.ts --- src/types.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/types.ts b/src/types.ts index 2119c6a2..70936054 100644 --- a/src/types.ts +++ b/src/types.ts @@ -126,10 +126,7 @@ export type State = StrategyOptions & { request: OctokitTypes.RequestInterface; cache: Cache; log: { - debug: (message: string, additionalInfo?: object) => any; - info: (message: string, additionalInfo?: object) => any; warn: (message: string, additionalInfo?: object) => any; - error: (message: string, additionalInfo?: object) => any; [key: string]: any; }; };