Merge pull request #261 from matrix-org/hs/always-200-events

Always send a 200 on an Event API request

changelog.d/261.bugfix

@@ -0,0 +1 @@
+Fix issue where using RTM on large deployments would trigger Slack to turn off Event subscriptions

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) {

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})(?:\/(.*))?$/);

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`);
                 }

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;
     });
 });