Fix #1358 Expose common utilities for building HTTP module based receivers #1359

seratch · 2022-03-07T09:25:25Z

Summary

This pull request resolves #1358 by extracting many parts of the HTTPReceiver and enabling them to use for any external code. I will add a few in-line comments for reviewers. The newly adde Koa.js based receiver implementation demonstrates how to use the extracted common modules outside the bolt-js core code.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2022-03-07T09:27:32Z

Codecov Report

Merging #1359 (ad69eb4) into main (4e12423) will increase coverage by 4.42%.
The diff coverage is 57.50%.

@@            Coverage Diff             @@
##             main    #1359      +/-   ##
==========================================
+ Coverage   72.69%   77.12%   +4.42%     
==========================================
  Files          17       17              
  Lines        1465     1460       -5     
  Branches      437      432       -5     
==========================================
+ Hits         1065     1126      +61     
+ Misses        310      236      -74     
- Partials       90       98       +8

Impacted Files	Coverage Δ
src/errors.ts	`98.11% <ø> (ø)`
src/receivers/HTTPResponseAck.ts	`10.34% <10.34%> (ø)`
src/receivers/ExpressReceiver.ts	`67.58% <25.00%> (+6.88%)`	⬆️
src/receivers/HTTPReceiver.ts	`59.76% <27.77%> (+8.90%)`	⬆️
src/receivers/HTTPModuleFunctions.ts	`77.45% <77.45%> (ø)`
src/receivers/render-html-for-install-path.ts	`85.71% <100.00%> (+10.71%)`	⬆️
src/receivers/http-utils.ts
src/receivers/verify-request.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e12423...ad69eb4. Read the comment docs.

seratch

comments for reviewers

seratch · 2022-03-07T09:26:10Z

examples/custom-receiver/src/KoaReceiver.ts

+  ReceiverUnhandledRequestHandlerArgs,
+} from '@slack/bolt';
+
+// TODO: import from @slack/oauth


This issue will be resolved by slackapi/node-slack-sdk#1446

seratch · 2022-03-07T09:30:15Z

src/receivers/HTTPModuleFunctions.ts

+
+const verifyErrorPrefix = 'Failed to verify authenticity';
+
+export class HTTPModuleFunctions {


Just to have namespace to organize these functions, I've added this class and put all the functions as static methods to the class. I know some people may not prefer this style but importing a bunch of functions can be even harder. If you have a different idea to easily manage and use a large set of functions, let me know that!

🙋 person who does not prefer this style 😆
In all seriousness, I'm fine with it. Class-with-static-functions is basically the same thing as exporting an object of functions so it doesn't matter much to me. If we can avoid using instance-based class semantics (and particularly side effects), I think that makes maintainability and readability easier, so that is what is important to me.

seratch · 2022-03-07T09:31:21Z

src/receivers/HTTPModuleFunctions.ts

+  }
+}
+
+export interface RequestVerificationOptions {


Renamed this but the previous type name was not exposed to external code.

seratch · 2022-03-07T09:31:40Z

src/receivers/HTTPModuleFunctions.ts

+}
+
+// which handles errors occurred while dispatching a rqeuest
+export interface ReceiverDispatchErrorHandlerArgs {


Renamed this a bit (just removed HTTP prefix) but the previous type name was not exposed to external code.

seratch · 2022-03-07T09:34:15Z

src/receivers/HTTPReceiver.ts

-        }
-      }, this.unhandledRequestTimeoutMillis);
-
+      const ack = new HTTPResponseAck({


Some people may not like the idea to have a class object with state in general but we have many state / local variables (isAcknowledged, storedResponse, bufferedReq, res) here. Managing them in a single place should not be a bad idea.

While I prefer functional to object-oriented approach I really like it used for this situation! Inheritance for receivers and encapsulating shared logic in these classes makes a lot of sense to me.

seratch · 2022-03-07T09:34:42Z

src/receivers/HTTPResponseAck.ts

@@ -0,0 +1,88 @@
+import { Logger } from '@slack/logger';


TODO: write some simple unit tests for this class

seratch · 2022-03-07T09:34:54Z

src/receivers/HTTPResponseAck.ts

+    };
+  }
+
+  public markAsAcknowledged(): void {


naming suggestion?

ack()? acknowledge()? ACK()?

P.S. I'm bad at naming, don't listen to me.

Initially, I also named this as ack() but came to think that this method can be more specific about what the methods actually does. That being said, using ack() for simplicity here may be fine.

seratch · 2022-03-07T09:35:10Z

src/receivers/http-utils.ts

@@ -1,5 +1,6 @@
 import { IncomingMessage } from 'http';

+// Deprecated: this function will be removed in the near future
 export function extractRetryNum(req: IncomingMessage): number | undefined {


I didn't remove these methods this time. These methods are no longer used. Also, they are not exported from src/index.ts. Thus, deleting this whole file should be safe enough.

Perhaps we should add some kind of warning log to these deprecated exported methods? Otherwise for existing users upgrading this would be invisible.

Great idea. Will add some logs

seratch · 2022-03-07T09:38:19Z

examples/custom-receiver/src/KoaReceiver.ts

@@ -0,0 +1,335 @@
+/* eslint-disable node/no-extraneous-import */


I'm going to add one more example (perhaps, fastify) here. I'm planning that these receivers can be eventually released as optional npm packages.

filmaj

Wow what a great addition! The KoaReceiver is such a great instructional tool, too. I wish this existed when I started at Slack, it would have really helped me understand how Bolt works.

Very nicely done!

I think there will be a lot of documentation work related to this PR that would be an excellent reference for devs. Not just API reference docs for the shared methods you encapsulated into utilities for devs to use in their own receivers, but also I think a kind of tutorial or walk-through doc showing devs how to write a custom receiver from scratch (i.e. taking this existing document and turning it into a larger, more robust tutorial document).

examples/custom-receiver/src/KoaReceiver.ts

filmaj · 2022-03-07T18:49:56Z

examples/custom-receiver/src/KoaReceiver.ts

+      } catch (err) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const e = err as any;
+        this.logger.warn(`Request verification failed: ${e.message}`);


Hmm, in the case that signature verification is disabled, would this be the right error message to surface?

This error message was taken from HTTPReceiver - we can improve both

examples/custom-receiver/src/app.ts

filmaj · 2022-03-07T18:56:08Z

examples/custom-receiver/src/KoaReceiver.ts

+  installationStore?: InstallProviderOptions['installationStore']; // default MemoryInstallationStore
+  scopes?: InstallURLOptions['scopes'];
+  installerOptions?: InstallerOptions;
+  koa: Koa;


What do you think about making the koa and router properties here optional?

Good point - I will make this more flexible later

filmaj · 2022-03-07T19:52:02Z

src/receivers/ExpressReceiver.ts

@@ -264,55 +264,25 @@ export default class ExpressReceiver implements Receiver {
  }

  private async requestHandler(req: Request, res: Response): Promise<void> {


Wow what a huge improvement!

filmaj · 2022-03-07T19:54:25Z

src/receivers/HTTPModuleFunctions.ts

+
+const verifyErrorPrefix = 'Failed to verify authenticity';
+
+export class HTTPModuleFunctions {


🙋 person who does not prefer this style 😆
In all seriousness, I'm fine with it. Class-with-static-functions is basically the same thing as exporting an object of functions so it doesn't matter much to me. If we can avoid using instance-based class semantics (and particularly side effects), I think that makes maintainability and readability easier, so that is what is important to me.

filmaj · 2022-03-07T19:56:51Z

src/receivers/HTTPReceiver.ts

-        }
-      }, this.unhandledRequestTimeoutMillis);
-
+      const ack = new HTTPResponseAck({


While I prefer functional to object-oriented approach I really like it used for this situation! Inheritance for receivers and encapsulating shared logic in these classes makes a lot of sense to me.

filmaj · 2022-03-07T19:59:19Z

src/receivers/HTTPResponseAck.ts

+    };
+  }
+
+  public markAsAcknowledged(): void {


ack()? acknowledge()? ACK()?

P.S. I'm bad at naming, don't listen to me.

filmaj · 2022-03-07T20:00:00Z

src/receivers/http-utils.ts

@@ -1,5 +1,6 @@
 import { IncomingMessage } from 'http';

+// Deprecated: this function will be removed in the near future
 export function extractRetryNum(req: IncomingMessage): number | undefined {


Perhaps we should add some kind of warning log to these deprecated exported methods? Otherwise for existing users upgrading this would be invisible.

danerwilliams

Hey there! Very excited for this PR 😄

Currently looking through the code to use before these functions are available in the package, and noticed a couple of things that stood out to me. Hope these comments are helpful.

danerwilliams · 2022-03-07T22:21:58Z

src/receivers/HTTPModuleFunctions.ts

+    const hmac = createHmac('sha256', signingSecret);
+    hmac.update(`${signatureVersion}:${requestTimestampSec}:${bufferedReq.rawBody.toString()}`);
+    const ourSignatureHash = hmac.digest('hex');
+    if (!tsscmp(signatureHash, ourSignatureHash)) {


Does this throw a type error? signatureHash could be undefined.

Probably? We can add more tests to check this

danerwilliams · 2022-03-07T22:28:26Z

src/receivers/HTTPModuleFunctions.ts

+  public static getHeader(req: IncomingMessage, header: string): string {
+    const value = req.headers[header];
+    if (value === undefined || Array.isArray(value)) {
+      throw new Error(`${verifyErrorPrefix}: header ${header} did not have the expected type (${value})`);


inside of this conditional we know that value will be either undefined or array which is not the expected type to insert into the formatted string.

I think that this message also is not entirely clear about the error as is.

What would you think about something like ... the expected type (received ${typeof value}, expected string)

Sounds good 👍 I would like to avoid changing these functions a lot in this PR but error message improvement would be safe enough.

seratch · 2022-03-08T22:34:52Z

Before merging this pull request, I am going to work on the following changes:

Do more tests with HTTPReceiver and ExpressReceiver to make sure if there is no regression bug
Add more unit tests to cover the extracted functions
Add more example receivers in the examples/custom-receiver app

examples/custom-receiver/README.md

…sed receivers

Co-authored-by: Fil Maj <[email protected]>

seratch added the enhancement M-T: A feature request for new functionality label Mar 7, 2022

seratch added this to the 3.11.0 milestone Mar 7, 2022

seratch requested review from filmaj, stevengill and srajiang March 7, 2022 09:25

seratch self-assigned this Mar 7, 2022

seratch commented Mar 7, 2022

View reviewed changes

filmaj approved these changes Mar 7, 2022

View reviewed changes

danerwilliams reviewed Mar 7, 2022

View reviewed changes

danerwilliams mentioned this pull request Mar 23, 2022

Create a signature validation function that is not tied to the request #1385

Closed

10 tasks

seratch force-pushed the issue-1358-http-functions branch from d8cf79f to 82eda54 Compare March 25, 2022 13:54

seratch commented Mar 25, 2022

View reviewed changes

examples/custom-receiver/README.md Outdated Show resolved Hide resolved

examples/custom-receiver/README.md Outdated Show resolved Hide resolved

seratch and others added 9 commits March 26, 2022 09:25

Fix slackapi#1358 Expose common utilities for building HTTP module ba…

6930377

…sed receivers

Apply suggestions from code review

5a61911

Co-authored-by: Fil Maj <[email protected]>

Update examples/custom-receiver/src/app.ts

c58533d

Co-authored-by: Fil Maj <[email protected]>

Update the PR reflecting the review comments

943bd2b

Update using oauth v2.5.0

727895e

Apply suggestions from code review

9dba877

Add unit tests

6103cea

Fix tests

98bcb71

Add more tests

1c815d9

seratch force-pushed the issue-1358-http-functions branch from 4faf1b6 to 1c815d9 Compare March 26, 2022 00:25

Add fastify example

ad69eb4

seratch merged commit be526eb into slackapi:main Mar 26, 2022

seratch deleted the issue-1358-http-functions branch March 26, 2022 03:34

seratch mentioned this pull request Jul 25, 2022

Fix #1509 HTTPReceiver does not immediately respond to an invalid signature request (no response instead) #1528

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1358 Expose common utilities for building HTTP module based receivers #1359

Fix #1358 Expose common utilities for building HTTP module based receivers #1359

seratch commented Mar 7, 2022

codecov bot commented Mar 7, 2022 •

edited

Loading

seratch left a comment

seratch Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

seratch Mar 7, 2022

seratch Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

seratch Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

seratch Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

seratch Mar 7, 2022

seratch Mar 7, 2022

filmaj left a comment

filmaj Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

seratch Mar 7, 2022

filmaj Mar 7, 2022

filmaj Mar 7, 2022

filmaj Mar 7, 2022

filmaj Mar 7, 2022

filmaj Mar 7, 2022

danerwilliams left a comment

danerwilliams Mar 7, 2022

seratch Mar 7, 2022

danerwilliams Mar 7, 2022

seratch Mar 7, 2022

seratch commented Mar 8, 2022


		const verifyErrorPrefix = 'Failed to verify authenticity';

		export class HTTPModuleFunctions {

		@@ -0,0 +1,335 @@
		/* eslint-disable node/no-extraneous-import */

		@@ -264,55 +264,25 @@ export default class ExpressReceiver implements Receiver {
		}

		private async requestHandler(req: Request, res: Response): Promise<void> {

Fix #1358 Expose common utilities for building HTTP module based receivers #1359

Fix #1358 Expose common utilities for building HTTP module based receivers #1359

Conversation

seratch commented Mar 7, 2022

Summary

Requirements (place an x in each [ ])

codecov bot commented Mar 7, 2022 • edited Loading

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danerwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Mar 8, 2022

Requirements (place an `x` in each `[ ]`)

codecov bot commented Mar 7, 2022 •

edited

Loading