Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #759 #1109 #1110 by adding custom properties in ReceiverEvent and Context objets #1177

Merged
merged 3 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/custom-properties/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
node_modules/
package-lock.json
21 changes: 21 additions & 0 deletions examples/custom-properties/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2021 Slack Technologies, LLC

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
42 changes: 42 additions & 0 deletions examples/custom-properties/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const { App, HTTPReceiver, ExpressReceiver } = require('@slack/bolt');

const useExpress = false;

let receiver;
receiver = new HTTPReceiver({
signingSecret: process.env.SLACK_SIGNING_SECRET,
customPropertiesExtractor: (req) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this name is a bit verbose. Does anyone have a great idea for the naming?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine!

return {
"headers": req.headers,
"foo": "bar",
};
}
});
if (useExpress) {
receiver = new ExpressReceiver({
signingSecret: process.env.SLACK_SIGNING_SECRET,
customPropertiesExtractor: (req) => {
return {
"headers": req.headers,
"foo": "bar",
};
}
});
}

const app = new App({
token: process.env.SLACK_BOT_TOKEN,
receiver,
});

app.use(async ({ logger, context, next }) => {
logger.info(context);
await next();
});

(async () => {
// Start your app
await app.start(process.env.PORT || 3000);

console.log('⚡️ Bolt app is running!');
})();
11 changes: 11 additions & 0 deletions examples/custom-properties/link.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/bash

current_dir=`dirname $0`
cd ${current_dir}
npm unlink @slack/bolt \
&& npm i \
&& cd ../.. \
&& npm link \
&& cd - \
&& npm i \
&& npm link @slack/bolt
13 changes: 13 additions & 0 deletions examples/custom-properties/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "bolt-js-custom-properties-app",
"version": "1.0.0",
"description": "Having custom request properties in ⚡️ Bolt for JavaScript",
"main": "app.js",
"scripts": {
"ngrok": "ngrok http 3000",
"start": "node app.js",
"test": "echo \"Error: no test specified\" && exit 1"
},
"license": "MIT",
"dependencies": {}
}
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@
"dependencies": {
"@slack/logger": "^3.0.0",
"@slack/oauth": "^2.3.0",
"@slack/socket-mode": "^1.1.0",
"@slack/socket-mode": "^1.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for extracting retry_num and retry_attempt value from payload

"@slack/types": "^2.2.0",
"@slack/web-api": "^6.4.0",
"@types/express": "^4.16.1",
"@types/node": ">=12",
"@types/promise.allsettled": "^1.0.3",
"@types/tsscmp": "^1.0.0",
"axios": "^0.21.2",
"axios": "^0.21.4",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm audit fix

"express": "^4.16.4",
"please-upgrade-node": "^3.2.0",
"promise.allsettled": "^1.0.2",
Expand Down
20 changes: 17 additions & 3 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ import {
WorkflowStepEdit,
} from './types';
import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers';
import { CodedError, asCodedError, AppInitializationError, MultipleListenerError, ErrorCode } from './errors';
import { AllMiddlewareArgs } from './types/middleware';
import { CodedError, asCodedError, AppInitializationError, MultipleListenerError, ErrorCode, InvalidCustomPropertyError } from './errors';
import { AllMiddlewareArgs, contextBuiltinKeys } from './types/middleware';
import { StringIndexed } from './types/helpers';
// eslint-disable-next-line import/order
import allSettled = require('promise.allsettled'); // eslint-disable-line @typescript-eslint/no-require-imports
// eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-commonjs
Expand Down Expand Up @@ -768,7 +769,20 @@ export default class App {
authorizeResult.enterpriseId = source.enterpriseId;
}

const context: Context = { ...authorizeResult };
if (typeof event.customProperties !== 'undefined') {
const customProps: StringIndexed = event.customProperties;
const builtinKeyDetected = contextBuiltinKeys.find((key) => key in customProps);
if (typeof builtinKeyDetected !== 'undefined') {
throw new InvalidCustomPropertyError('customProperties cannot have the same names with the built-in ones');
seratch marked this conversation as resolved.
Show resolved Hide resolved
}
}

const context: Context = {
...authorizeResult,
...event.customProperties,
retryNum: event.retryNum,
retryReason: event.retryReason,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of explicitly adding the retryNum and retryReason keys to the context object here? It looks like these two keys are part of the "built in" keys, so the check above should throw is these are reused by the custom properties, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two properties are built-in ones but they are supposed to be sent as part of the receiver event data, not from the authorize function. Thus, we set these this way.

It looks like these two keys are part of the "built in" keys, so the check above should throw is these are reused by the custom properties, right?

Yes, your understanding here is correct.

};

// Factory for say() utility
const createSay = (channelId: string): SayFn => {
Expand Down
5 changes: 5 additions & 0 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export enum ErrorCode {
AuthorizationError = 'slack_bolt_authorization_error',

ContextMissingPropertyError = 'slack_bolt_context_missing_property_error',
InvalidCustomPropertyError = 'slack_bolt_context_invalid_custom_property_error',

CustomRouteInitializationError = 'slack_bolt_custom_route_initialization_error',

Expand Down Expand Up @@ -82,6 +83,10 @@ export class ContextMissingPropertyError extends Error implements CodedError {
}
}

export class InvalidCustomPropertyError extends Error implements CodedError {
public code = ErrorCode.AppInitializationError;
}

export class CustomRouteInitializationError extends Error implements CodedError {
public code = ErrorCode.CustomRouteInitializationError;
}
Expand Down
4 changes: 4 additions & 0 deletions src/receivers/AwsLambdaReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,10 @@ export default class AwsLambdaReceiver implements Receiver {
storedResponse = response;
}
},
retryNum: this.getHeaderValue(awsEvent.headers, 'X-Slack-Retry-Num') as number | undefined,
retryReason: this.getHeaderValue(awsEvent.headers, 'X-Slack-Retry-Reason'),
// TODO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

customProperties: {},
};

// Send the event to the app for processing
Expand Down
11 changes: 11 additions & 0 deletions src/receivers/ExpressReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
import { AnyMiddlewareArgs, Receiver, ReceiverEvent } from '../types';
import defaultRenderHtmlForInstallPath from './render-html-for-install-path';
import { verifyRedirectOpts } from './verify-redirect-opts';
import { StringIndexed } from '../types/helpers';
import { extractRetryNum, extractRetryReason } from './http-utils';

// Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer()
const httpsOptionKeys = [
Expand Down Expand Up @@ -95,6 +97,7 @@ export interface ExpressReceiverOptions {
installerOptions?: InstallerOptions;
app?: Application;
router?: IRouter;
customPropertiesExtractor?: (request: Request) => StringIndexed;
}

// Additional Installer Options
Expand Down Expand Up @@ -136,6 +139,8 @@ export default class ExpressReceiver implements Receiver {

public installerOptions?: InstallerOptions;

private customPropertiesExtractor: (request: Request) => StringIndexed;

public constructor({
signingSecret = '',
logger = undefined,
Expand All @@ -152,6 +157,7 @@ export default class ExpressReceiver implements Receiver {
installerOptions = {},
app = undefined,
router = undefined,
customPropertiesExtractor = () => ({}),
}: ExpressReceiverOptions) {
this.app = app !== undefined ? app : express();

Expand Down Expand Up @@ -180,6 +186,8 @@ export default class ExpressReceiver implements Receiver {
this.router.post(endpoint, ...expressMiddleware);
});

this.customPropertiesExtractor = customPropertiesExtractor;

// Verify redirect options if supplied, throws coded error if invalid
verifyRedirectOpts({ redirectUri, redirectUriPath: installerOptions.redirectUriPath });

Expand Down Expand Up @@ -292,6 +300,9 @@ export default class ExpressReceiver implements Receiver {
this.logger.debug('ack() response sent');
}
},
retryNum: extractRetryNum(req),
retryReason: extractRetryReason(req),
customProperties: this.customPropertiesExtractor(req),
};

try {
Expand Down
10 changes: 10 additions & 0 deletions src/receivers/HTTPReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
CodedError,
} from '../errors';
import { CustomRoute, prepareRoutes, ReceiverRoutes } from './custom-routes';
import { StringIndexed } from '../types/helpers';
import { extractRetryNum, extractRetryReason } from './http-utils';

// Option keys for tls.createServer() and tls.createSecureContext(), exclusive of those for http.createServer()
const httpsOptionKeys = [
Expand Down Expand Up @@ -72,6 +74,7 @@ export interface HTTPReceiverOptions {
installationStore?: InstallProviderOptions['installationStore']; // default MemoryInstallationStore
scopes?: InstallURLOptions['scopes'];
installerOptions?: HTTPReceiverInstallerOptions;
customPropertiesExtractor?: (request: BufferedIncomingMessage) => StringIndexed;
}
export interface HTTPReceiverInstallerOptions {
installPath?: string;
Expand Down Expand Up @@ -126,6 +129,8 @@ export default class HTTPReceiver implements Receiver {

private logger: Logger;

private customPropertiesExtractor: (request: BufferedIncomingMessage) => StringIndexed;

public constructor({
signingSecret = '',
endpoints = ['/slack/events'],
Expand All @@ -141,6 +146,7 @@ export default class HTTPReceiver implements Receiver {
installationStore = undefined,
scopes = undefined,
installerOptions = {},
customPropertiesExtractor = () => ({}),
}: HTTPReceiverOptions) {
// Initialize instance variables, substituting defaults for each value
this.signingSecret = signingSecret;
Expand Down Expand Up @@ -196,6 +202,7 @@ export default class HTTPReceiver implements Receiver {
this.renderHtmlForInstallPath = installerOptions.renderHtmlForInstallPath !== undefined ?
installerOptions.renderHtmlForInstallPath :
defaultRenderHtmlForInstallPath;
this.customPropertiesExtractor = customPropertiesExtractor;

// Assign the requestListener property by binding the unboundRequestListener to this instance
this.requestListener = this.unboundRequestListener.bind(this);
Expand Down Expand Up @@ -439,6 +446,9 @@ export default class HTTPReceiver implements Receiver {
this.logger.debug('ack() response sent');
}
},
retryNum: extractRetryNum(req),
retryReason: extractRetryReason(req),
customProperties: this.customPropertiesExtractor(bufferedReq),
};

// Send the event to the app for processing
Expand Down
6 changes: 6 additions & 0 deletions src/receivers/SocketModeReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { AppsConnectionsOpenResponse } from '@slack/web-api';
import App from '../App';
import { Receiver, ReceiverEvent } from '../types';
import defaultRenderHtmlForInstallPath from './render-html-for-install-path';
import { StringIndexed } from '../types/helpers';
import { prepareRoutes, ReceiverRoutes } from './custom-routes';
import { verifyRedirectOpts } from './verify-redirect-opts';

Expand All @@ -24,6 +25,7 @@ export interface SocketModeReceiverOptions {
installerOptions?: InstallerOptions;
appToken: string; // App Level Token
customRoutes?: CustomRoute[];
customPropertiesExtractor?: (request: any) => StringIndexed;
}

export interface CustomRoute {
Expand Down Expand Up @@ -76,6 +78,7 @@ export default class SocketModeReceiver implements Receiver {
scopes = undefined,
installerOptions = {},
customRoutes = [],
customPropertiesExtractor = (_req) => ({}),
}: SocketModeReceiverOptions) {
this.client = new SocketModeClient({
appToken,
Expand Down Expand Up @@ -207,6 +210,9 @@ export default class SocketModeReceiver implements Receiver {
const event: ReceiverEvent = {
body,
ack,
retryNum: body.retry_attempt,
retryReason: body.retry_reason,
customProperties: customPropertiesExtractor(body),
};
await this.app?.processEvent(event);
});
Expand Down
28 changes: 28 additions & 0 deletions src/receivers/http-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { IncomingMessage } from 'http';

export function extractRetryNum(req: IncomingMessage): number | undefined {
let retryNum;
const retryNumHeaderValue = req.headers['x-slack-retry-num'];
if (retryNumHeaderValue === undefined) {
retryNum = undefined;
} else if (typeof retryNumHeaderValue === 'string') {
retryNum = parseInt(retryNumHeaderValue, 10);
} else if (Array.isArray(retryNumHeaderValue) && retryNumHeaderValue.length > 0) {
retryNum = parseInt(retryNumHeaderValue[0], 10);
}
return retryNum;
}

export function extractRetryReason(req: IncomingMessage): string | undefined {
let retryReason;
const retryReasonHeaderValue = req.headers['x-slack-retry-reason'];
if (retryReasonHeaderValue === undefined) {
retryReason = undefined;
} else if (typeof retryReasonHeaderValue === 'string') {
retryReason = retryReasonHeaderValue;
} else if (Array.isArray(retryReasonHeaderValue) && retryReasonHeaderValue.length > 0) {
// eslint-disable-next-line prefer-destructuring
retryReason = retryReasonHeaderValue[0];
}
return retryReason;
}
20 changes: 20 additions & 0 deletions src/types/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ export interface Context extends StringIndexed {
* Enterprise Grid Organization ID.
*/
enterpriseId?: string;

/**
* Retry count of an Events API request (this property does not exist for other requests)
*/
retryNum?: number;
/**
* Retry reason of an Events API request (this property does not exist for other requests)
*/
retryReason?: string;
}

export const contextBuiltinKeys: string[] = [
'botToken',
'userToken',
'botId',
'botUserId',
'teamId',
'enterpriseId',
'retryNum',
'retryReason',
];

export type NextFn = () => Promise<void>;
18 changes: 16 additions & 2 deletions src/types/receiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,23 @@ import { AckFn } from './index';
import { StringIndexed } from './helpers';

export interface ReceiverEvent {
// Parsed HTTP request body / Socket Mode message body
body: StringIndexed;
// TODO: there should maybe be some more help for implementors of Receiver to know what kind of argument the AckFn
// is expected to deal with.

// X-Slack-Retry-Num: 2 in HTTP Mode
// "retry_attempt": 0, in Socket Mode
retryNum?: number;

// X-Slack-Retry-Reason: http_error in HTTP Mode
// "retry_reason": "timeout", in Socket Mode
retryReason?: string;

// Custom properties like HTTP request headers
customProperties?: StringIndexed;

// The function to acknowledge incoming requests
// The details of implementation is encapsulated in a receiver
// TODO: Make the argument type more specific
ack: AckFn<any>;
}

Expand Down