From 7a5ad1284fc279b566bdfb09d3f52fbd28b0ffd4 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Tue, 24 Sep 2019 19:13:14 +0100 Subject: [PATCH 1/5] Fix team upsert --- src/SlackHookHandler.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/SlackHookHandler.ts b/src/SlackHookHandler.ts index 062b97b1..d2df60c3 100644 --- a/src/SlackHookHandler.ts +++ b/src/SlackHookHandler.ts @@ -335,17 +335,11 @@ export class SlackHookHandler extends BaseSlackHandler { response.bot === undefined, ); if (response.bot) { - const team: TeamEntry = { - bot_token: response.bot!.bot_access_token, - bot_id: response.bot!.bot_user_id, - user_id: "unknown", - id: response.team_id, - name: response.team_name, - scopes: access_scopes.join(","), - status: "ok", - domain: "not-set", - }; - await this.main.datastore.upsertTeam(team); + // Rather than upsert the values we were given, use the + // access token to validate and make additional requests + await this.main.clientFactory.upsertTeamByToken( + response.bot.bot_access_token, + ); } } } catch (err) { From f22ed61dc06184595b2d3bb944839a2efb6e491f Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Tue, 24 Sep 2019 19:15:28 +0100 Subject: [PATCH 2/5] Newsfile --- changelog.d/260.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/260.bugfix diff --git a/changelog.d/260.bugfix b/changelog.d/260.bugfix new file mode 100644 index 00000000..bffde0e8 --- /dev/null +++ b/changelog.d/260.bugfix @@ -0,0 +1 @@ +Fix bug where teams would start echoing messages after accepting an oauth request. \ No newline at end of file From 97486d40a721031a04f2952650aa8daa80dedc08 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 25 Sep 2019 09:48:05 +0100 Subject: [PATCH 3/5] Refactor Event API handling so that we always 200 OK --- src/SlackEventHandler.ts | 16 +++++++--- src/SlackHookHandler.ts | 68 +++++++++++++++++++++------------------- src/SlackRTMHandler.ts | 2 +- 3 files changed, 48 insertions(+), 38 deletions(-) diff --git a/src/SlackEventHandler.ts b/src/SlackEventHandler.ts index 38dec57c..0b212432 100644 --- a/src/SlackEventHandler.ts +++ b/src/SlackEventHandler.ts @@ -66,15 +66,23 @@ export class SlackEventHandler extends BaseSlackHandler { * Handles a slack event request. * @param ISlackEventParams */ - public async handle(event: ISlackEvent, teamId: string, response: EventHandlerCallback) { + public async handle(event: ISlackEvent, teamId: string, response: EventHandlerCallback, isEventAndUsingRtm: boolean) { try { - log.debug("Received slack event:", event, teamId); - - const endTimer = this.main.startTimer("remote_request_seconds"); // See https://api.slack.com/events-api#responding_to_events // We must respond within 3 seconds or it will be sent again! response(HTTP_OK, "OK"); + if (isEventAndUsingRtm) { + // This is a special flag that is raised if the team is using the RTM + // API AND this event is from the Events API. Certain Events only come down + // that API, and we may have to handle those in the future. For now, all the events + // given below can be found on both APIs. + // If this flag is true, we should return early to avoid duplication. + return; + } + log.debug("Received slack event:", event, teamId); + const endTimer = this.main.startTimer("remote_request_seconds"); + let err: Error|null = null; try { switch (event.type) { diff --git a/src/SlackHookHandler.ts b/src/SlackHookHandler.ts index d2df60c3..01367d22 100644 --- a/src/SlackHookHandler.ts +++ b/src/SlackHookHandler.ts @@ -99,39 +99,10 @@ export class SlackHookHandler extends BaseSlackHandler { const isEvent = req.headers["content-type"] === "application/json" && req.method === "POST"; try { if (isEvent) { - const eventsResponse = (resStatus, resBody, resHeaders) => { - if (resHeaders) { - res.writeHead(resStatus, resHeaders); - } else { - res.writeHead(resStatus); - } - if (resBody) { - res.write(resBody); - } - res.end(); - }; - const eventPayload = JSON.parse(body) as ISlackEventPayload; - if (eventPayload.type === "url_verification") { - this.eventHandler.onVerifyUrl(eventPayload.challenge!, eventsResponse); - // If RTM is enabled for this team, do not handle the event. - // Slack will push us events for all connected teams to our bots, - // but this will cause duplicates if the team is ALSO using RTM. - } else if (eventPayload.type === "event_callback" && - !this.main.teamIsUsingRtm(eventPayload.team_id.toUpperCase()) - ) { - this.eventHandler.handle( - // The event can take many forms. - // tslint:disable-next-line: no-any - eventPayload.event as any, - eventPayload.team_id, - eventsResponse, - ).catch((ex) => { - log.error("Failed to handle event", ex); - }); - } + this.handleEvent(body, res); } else { const params = qs.parse(body); - this.handle(req.method!, req.url!, params, res).catch((ex) => { + this.handleWebhook(req.method!, req.url!, params, res).catch((ex) => { log.error("Failed to handle webhook event", ex); }); } @@ -146,6 +117,37 @@ export class SlackHookHandler extends BaseSlackHandler { }); } + private handleEvent(jsonBodyStr: string, res: ServerResponse) { + const eventsResponse = (resStatus: number, resBody?: string, resHeaders?: {[key: string]: string}) => { + if (resHeaders) { + res.writeHead(resStatus, resHeaders); + } else { + res.writeHead(resStatus); + } + if (resBody) { + res.write(resBody); + } + res.end(); + }; + const eventPayload = JSON.parse(jsonBodyStr) as ISlackEventPayload; + if (eventPayload.type === "url_verification") { + this.eventHandler.onVerifyUrl(eventPayload.challenge!, eventsResponse); + } else if (eventPayload.type !== "event_callback") { + return; // We can't handle anything else. + } + const isUsingRtm = this.main.teamIsUsingRtm(eventPayload.team_id.toUpperCase()); + this.eventHandler.handle( + // The event can take many forms. + // tslint:disable-next-line: no-any + eventPayload.event as any, + eventPayload.team_id, + eventsResponse, + isUsingRtm, + ).catch((ex) => { + log.error("Failed to handle event", ex); + }); + } + /** * Handles a slack webhook request. * @@ -155,8 +157,8 @@ export class SlackHookHandler extends BaseSlackHandler { * @param url The HTTP url for the incoming request * @param params Parameters given in either the body or query string. */ - private async handle(method: string, url: string, params: {[key: string]: string|string[]}, - response: ServerResponse) { + private async handleWebhook(method: string, url: string, params: {[key: string]: string|string[]}, + response: ServerResponse) { log.info(`Received slack webhook ${method} ${url}: ${JSON.stringify(params)}`); const endTimer = this.main.startTimer("remote_request_seconds"); const urlMatch = url.match(/^\/(.{32})(?:\/(.*))?$/); diff --git a/src/SlackRTMHandler.ts b/src/SlackRTMHandler.ts index ed4aa050..a3a468f3 100644 --- a/src/SlackRTMHandler.ts +++ b/src/SlackRTMHandler.ts @@ -127,7 +127,7 @@ export class SlackRTMHandler extends SlackEventHandler { log.error("Cannot handle event, no active teamId!"); return; } - await this.handle(event, rtm.activeTeamId! , () => {}); + await this.handle(event, rtm.activeTeamId! , () => {}, false); } catch (ex) { log.error(`Failed to handle '${eventName}' event`); } From 1baac340187c6c2775cb86e77a0c1db32996122e Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 25 Sep 2019 09:48:28 +0100 Subject: [PATCH 4/5] Add test to ensure we always 200 the response --- src/tests/integration/SlackToMatrixTest.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/tests/integration/SlackToMatrixTest.ts b/src/tests/integration/SlackToMatrixTest.ts index 8e486175..04a0f4f5 100644 --- a/src/tests/integration/SlackToMatrixTest.ts +++ b/src/tests/integration/SlackToMatrixTest.ts @@ -36,14 +36,32 @@ describe("SlackToMatrix", () => { }); it("will drop slack events that have an unknown type", async () => { + let called = false; await harness.eventHandler.handle({ type: "faketype", channel: "fakechannel", ts: "12345", }, "12345", (status: number, body?: string) => { + called = true; expect(status).to.equal(200); expect(body).to.equal("OK"); - }); + }, false); expect(harness.main.timerFinished.remote_request_seconds).to.be.equal("dropped"); + expect(called).to.be.true; + }); + + it("will no-op slack events when using RTM API and is an Event API request", async () => { + let called = false; + await harness.eventHandler.handle({ + type: "faketype", + channel: "fakechannel", + ts: "12345", + }, "12345", (status: number, body?: string) => { + called = true; + expect(status).to.equal(200); + expect(body).to.equal("OK"); + }, true); + expect(harness.main.timerFinished.remote_request_seconds).to.be.undefined; + expect(called).to.be.true; }); }); From 02d3298bf7d6b26dbca79ee01e42a43a1d542e91 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 25 Sep 2019 09:51:10 +0100 Subject: [PATCH 5/5] add newsfile --- changelog.d/261.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/261.bugfix diff --git a/changelog.d/261.bugfix b/changelog.d/261.bugfix new file mode 100644 index 00000000..a5ae7aa1 --- /dev/null +++ b/changelog.d/261.bugfix @@ -0,0 +1 @@ +Fix issue where using RTM on large deployments would trigger Slack to turn off Event subscriptions \ No newline at end of file