From d459b8d0d69a5fe256b90883788a4457adac4d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=C3=A9ry=20Herlaud?= Date: Thu, 18 Mar 2021 14:13:30 +0100 Subject: [PATCH 1/5] fix(App): Can't catch errors from `singleAuthorization`. --- src/App.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/App.ts b/src/App.ts index f873a7cc8..c0d66e56b 100644 --- a/src/App.ts +++ b/src/App.ts @@ -1057,18 +1057,22 @@ function singleAuthorization( authorization: Partial & { botToken: Required['botToken'] }, ): Authorize { // TODO: warn when something needed isn't found - const identifiers: Promise<{ botUserId: string; botId: string }> = - authorization.botUserId !== undefined && authorization.botId !== undefined - ? Promise.resolve({ botUserId: authorization.botUserId, botId: authorization.botId }) - : client.auth.test({ token: authorization.botToken }).then((result) => { - return { - botUserId: result.user_id as string, - botId: result.bot_id as string, - }; - }); + let identifiers: { botUserId: string; botId: string }; return async ({ isEnterpriseInstall }) => { - return { isEnterpriseInstall, botToken: authorization.botToken, ...(await identifiers) }; + if (identifiers === undefined) { + identifiers = + authorization.botUserId !== undefined && authorization.botId !== undefined + ? { botUserId: authorization.botUserId, botId: authorization.botId } + : await client.auth.test({ token: authorization.botToken }).then((result) => { + return { + botUserId: result.user_id as string, + botId: result.bot_id as string, + }; + }); + } + + return { isEnterpriseInstall, botToken: authorization.botToken, ...identifiers }; }; } From 5a78060f97da6f3dd2d8b14da74cdaf3f064b8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=C3=A9ry=20Herlaud?= Date: Sun, 21 Mar 2021 19:18:42 +0100 Subject: [PATCH 2/5] feat(App): Add tests. --- src/App.spec.ts | 37 +++++++++++++++++++++++++++++++++++++ src/App.ts | 3 ++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 3e4d66da0..f0f38dcb7 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -55,6 +55,31 @@ describe('App', () => { assert.instanceOf(app, App); }); }); + + describe('with unsuccessful single team authorization results', () => { + it.only('should throws', async () => { + // Arrange + const error = new Error("An API error occurred: something's wrong"); + const overrides = mergeOverrides(withNoopAppMetadata(), withUnsuccessfulBotUserFetchingWebClient(error)); + const App = await importApp(overrides); // eslint-disable-line @typescript-eslint/naming-convention, no-underscore-dangle, id-blacklist, id-match + const event: ReceiverEvent = { + ack: async () => {}, + body: { + type: 'shortcut', + team: { id: '' }, + enterprise: { id: '' }, + user: { id: '' }, + }, + }; + + // Act + const app = new App({ token: '', signingSecret: '' }); + + // Assert + assert.equal(await app.processEvent(event).catch((e) => e as Error), error); + assert.equal(await app.processEvent(event).catch((e) => e as Error), error); // retry + }); + }); it('should succeed with an authorize callback', async () => { // Arrange const authorizeCallback = sinon.fake(); @@ -1915,6 +1940,18 @@ function withSuccessfulBotUserFetchingWebClient(botId: string, botUserId: string }; } +function withUnsuccessfulBotUserFetchingWebClient(error: Error): Override { + return { + '@slack/web-api': { + WebClient: class { + public auth = { + test: sinon.fake.rejects(error), + }; + }, + }, + }; +} + function withPostMessage(spy: SinonSpy): Override { return { '@slack/web-api': { diff --git a/src/App.ts b/src/App.ts index c0d66e56b..b48904efb 100644 --- a/src/App.ts +++ b/src/App.ts @@ -1064,7 +1064,8 @@ function singleAuthorization( identifiers = authorization.botUserId !== undefined && authorization.botId !== undefined ? { botUserId: authorization.botUserId, botId: authorization.botId } - : await client.auth.test({ token: authorization.botToken }).then((result) => { + : // While test is failing, the identifiers will not be assigned + await client.auth.test({ token: authorization.botToken }).then((result) => { return { botUserId: result.user_id as string, botId: result.bot_id as string, From 12f8c4f51d99f780c85f6b74c4f8f6964ea55eac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=C3=A9ry=20Herlaud?= Date: Sun, 21 Mar 2021 19:33:21 +0100 Subject: [PATCH 3/5] fix(App): Add fake logger to tests. --- src/App.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index f0f38dcb7..d4c3201d3 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -60,6 +60,7 @@ describe('App', () => { it.only('should throws', async () => { // Arrange const error = new Error("An API error occurred: something's wrong"); + const fakeLogger = createFakeLogger(); const overrides = mergeOverrides(withNoopAppMetadata(), withUnsuccessfulBotUserFetchingWebClient(error)); const App = await importApp(overrides); // eslint-disable-line @typescript-eslint/naming-convention, no-underscore-dangle, id-blacklist, id-match const event: ReceiverEvent = { @@ -73,11 +74,13 @@ describe('App', () => { }; // Act - const app = new App({ token: '', signingSecret: '' }); + const app = new App({ token: '', signingSecret: '', logger: fakeLogger }); // Assert assert.equal(await app.processEvent(event).catch((e) => e as Error), error); assert.equal(await app.processEvent(event).catch((e) => e as Error), error); // retry + assert.equal(fakeLogger.warn.callCount, 2); + assert.equal(fakeLogger.error.callCount, 2); }); }); it('should succeed with an authorize callback', async () => { From 606e6931a69171bb0ac3f3c7afb122190b0c8629 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=C3=A9ry=20Herlaud?= Date: Wed, 24 Mar 2021 14:16:49 +0100 Subject: [PATCH 4/5] fix(App): Remove only in test. --- src/App.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index d4c3201d3..48b019173 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -57,7 +57,7 @@ describe('App', () => { }); describe('with unsuccessful single team authorization results', () => { - it.only('should throws', async () => { + it('should throws', async () => { // Arrange const error = new Error("An API error occurred: something's wrong"); const fakeLogger = createFakeLogger(); From bb5eac8721e762c587e4baf0572eebdd094b5dc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=C3=A9ry=20Herlaud?= Date: Sat, 27 Mar 2021 21:53:41 +0100 Subject: [PATCH 5/5] feat(App): Add tokenVerificationEnabled in constructor arguments. --- src/App.spec.ts | 38 +++++++++++++++++++++++++--- src/App.ts | 66 +++++++++++++++++++++++++++++++++++-------------- 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 48b019173..ef597f731 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -57,11 +57,12 @@ describe('App', () => { }); describe('with unsuccessful single team authorization results', () => { - it('should throws', async () => { + it('should test once and throws multiple times when tokenVerificationEnabled is true', async () => { // Arrange const error = new Error("An API error occurred: something's wrong"); + const test = sinon.fake.rejects(error); const fakeLogger = createFakeLogger(); - const overrides = mergeOverrides(withNoopAppMetadata(), withUnsuccessfulBotUserFetchingWebClient(error)); + const overrides = mergeOverrides(withNoopAppMetadata(), withUnsuccessfulBotUserFetchingWebClient(test)); const App = await importApp(overrides); // eslint-disable-line @typescript-eslint/naming-convention, no-underscore-dangle, id-blacklist, id-match const event: ReceiverEvent = { ack: async () => {}, @@ -81,6 +82,35 @@ describe('App', () => { assert.equal(await app.processEvent(event).catch((e) => e as Error), error); // retry assert.equal(fakeLogger.warn.callCount, 2); assert.equal(fakeLogger.error.callCount, 2); + assert.equal(test.callCount, 1); + }); + + it('should test and throws multiple times when tokenVerificationEnabled is false', async () => { + // Arrange + const error = new Error("An API error occurred: something's wrong"); + const test = sinon.fake.rejects(error); + const fakeLogger = createFakeLogger(); + const overrides = mergeOverrides(withNoopAppMetadata(), withUnsuccessfulBotUserFetchingWebClient(test)); + const App = await importApp(overrides); // eslint-disable-line @typescript-eslint/naming-convention, no-underscore-dangle, id-blacklist, id-match + const event: ReceiverEvent = { + ack: async () => {}, + body: { + type: 'shortcut', + team: { id: '' }, + enterprise: { id: '' }, + user: { id: '' }, + }, + }; + + // Act + const app = new App({ token: '', signingSecret: '', logger: fakeLogger, tokenVerificationEnabled: false }); + + // Assert + assert.equal(await app.processEvent(event).catch((e) => e as Error), error); + assert.equal(await app.processEvent(event).catch((e) => e as Error), error); // retry + assert.equal(fakeLogger.warn.callCount, 2); + assert.equal(fakeLogger.error.callCount, 2); + assert.equal(test.callCount, 2); }); }); it('should succeed with an authorize callback', async () => { @@ -1943,12 +1973,12 @@ function withSuccessfulBotUserFetchingWebClient(botId: string, botUserId: string }; } -function withUnsuccessfulBotUserFetchingWebClient(error: Error): Override { +function withUnsuccessfulBotUserFetchingWebClient(test: SinonSpy): Override { return { '@slack/web-api': { WebClient: class { public auth = { - test: sinon.fake.rejects(error), + test, }; }, }, diff --git a/src/App.ts b/src/App.ts index b48904efb..d0f0a2f17 100644 --- a/src/App.ts +++ b/src/App.ts @@ -80,6 +80,7 @@ export interface AppOptions { clientOptions?: Pick; socketMode?: boolean; developerMode?: boolean; + tokenVerificationEnabled?: boolean; } export { LogLevel, Logger } from '@slack/logger'; @@ -210,6 +211,7 @@ export default class App { installerOptions = undefined, socketMode = undefined, developerMode = false, + tokenVerificationEnabled = true, }: AppOptions = {}) { // this.logLevel = logLevel; this.developerMode = developerMode; @@ -354,7 +356,21 @@ export default class App { `token as well as authorize or oauth installer options were provided. ${tokenUsage}`, ); } - this.authorize = singleAuthorization(this.client, { botId, botUserId, botToken: token }); + + if (botUserId !== undefined && botId !== undefined) { + this.authorize = ({ isEnterpriseInstall }) => + Promise.resolve({ + isEnterpriseInstall, + botId, + botUserId, + }); + } else { + this.authorize = singleAuthorizeFactory({ + client: this.client, + botToken: token, + tokenVerificationEnabled, + }); + } } else if (authorize === undefined && !usingOauth) { throw new AppInitializationError( `No token, no authorize, and no oauth installer options provided. ${tokenUsage}`, @@ -1052,28 +1068,42 @@ function defaultErrorHandler(logger: Logger): ErrorHandler { }; } -function singleAuthorization( - client: WebClient, - authorization: Partial & { botToken: Required['botToken'] }, -): Authorize { - // TODO: warn when something needed isn't found +function authTest(client: WebClient, token: string) { + return client.auth.test({ token }).then(({ user_id, bot_id }) => ({ + botUserId: user_id as string, + botId: bot_id as string, + })); +} + +interface SingleAuthorizeFactoryArgs { + client: WebClient; + tokenVerificationEnabled: boolean; + botToken: string; +} + +function singleAuthorizeFactory({ + client, + botToken, + tokenVerificationEnabled, +}: SingleAuthorizeFactoryArgs): App['authorize'] { let identifiers: { botUserId: string; botId: string }; + const authPromise = tokenVerificationEnabled ? authTest(client, botToken) : undefined; + return async ({ isEnterpriseInstall }) => { - if (identifiers === undefined) { - identifiers = - authorization.botUserId !== undefined && authorization.botId !== undefined - ? { botUserId: authorization.botUserId, botId: authorization.botId } - : // While test is failing, the identifiers will not be assigned - await client.auth.test({ token: authorization.botToken }).then((result) => { - return { - botUserId: result.user_id as string, - botId: result.bot_id as string, - }; - }); + if (!identifiers) { + if (authPromise) { + identifiers = await authPromise; + } else { + identifiers = await authTest(client, botToken); + } } - return { isEnterpriseInstall, botToken: authorization.botToken, ...identifiers }; + return { + isEnterpriseInstall, + botToken, + ...identifiers, + }; }; }