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

[v2] Convert receivers to use App#processEvent #380

Merged
merged 9 commits into from
Mar 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
325 changes: 219 additions & 106 deletions src/App.spec.ts

Large diffs are not rendered by default.

72 changes: 32 additions & 40 deletions src/App.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Agent } from 'http';
import { SecureContextOptions } from 'tls';
import util from 'util';
import {
WebClient,
Expand Down Expand Up @@ -44,16 +46,20 @@ import {
RespondArguments,
} from './types';
import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers';
import { ErrorCode, CodedError, errorWithCode, asCodedError } from './errors';
import {
CodedError,
asCodedError,
AppInitializationError,
} from './errors';
import { MiddlewareContext } from './types/middleware';
const packageJson = require('../package.json'); // tslint:disable-line:no-require-imports no-var-requires

/** App initialization options */
export interface AppOptions {
signingSecret?: ExpressReceiverOptions['signingSecret'];
endpoints?: ExpressReceiverOptions['endpoints'];
agent?: ExpressReceiverOptions['agent']; // also WebClientOptions['agent']
clientTls?: ExpressReceiverOptions['clientTls']; // also WebClientOptions['tls']
agent?: Agent;
clientTls?: Pick<SecureContextOptions, 'pfx' | 'key' | 'passphrase' | 'cert' | 'ca'>;
convoStore?: ConversationStore | false;
token?: AuthorizeResult['botToken']; // either token or authorize
botId?: AuthorizeResult['botId']; // only used when authorize is not defined, shortcut for fetching
Expand All @@ -72,7 +78,7 @@ export { LogLevel, Logger } from '@slack/logger';
export interface Authorize {
(
source: AuthorizeSourceData,
body: ReceiverEvent['body'],
body: AnyMiddlewareArgs['body'],
): Promise<AuthorizeResult>;
}

Expand Down Expand Up @@ -107,7 +113,7 @@ export interface ViewConstraints {
}

export interface ErrorHandler {
(error: CodedError): void;
(error: CodedError): Promise<void>;
}

class WebClientPool {
Expand Down Expand Up @@ -203,16 +209,14 @@ export default class App {

if (token !== undefined) {
if (authorize !== undefined) {
throw errorWithCode(
throw new AppInitializationError(
`Both token and authorize options provided. ${tokenUsage}`,
ErrorCode.AppInitializationError,
);
}
this.authorize = singleTeamAuthorization(this.client, { botId, botUserId, botToken: token });
} else if (authorize === undefined) {
throw errorWithCode(
throw new AppInitializationError(
`No token and no authorize options provided. ${tokenUsage}`,
ErrorCode.AppInitializationError,
);
} else {
this.authorize = authorize;
Expand All @@ -227,27 +231,20 @@ export default class App {
} else {
// No custom receiver
if (signingSecret === undefined) {
throw errorWithCode(
'Signing secret not found, so could not initialize the default receiver. Set a signing secret or use a ' +
'custom receiver.',
ErrorCode.AppInitializationError,
throw new AppInitializationError(
'Signing secret not found, so could not initialize the default receiver. Set a signing secret or use a ' +
'custom receiver.',
);
} else {
// Create default ExpressReceiver
this.receiver = new ExpressReceiver({
signingSecret,
endpoints,
agent,
clientTls,
logger: this.logger,
});
}
}

// Subscribe to messages and errors from the receiver
this.receiver.on('message', message => this.onIncomingEvent(message));
this.receiver.on('error', error => this.onGlobalError(error));

// Conditionally use a global middleware that ignores events (including messages) that are sent from this app
if (ignoreSelf) {
this.use(ignoreSelfMiddleware());
Expand All @@ -259,6 +256,9 @@ export default class App {
const store: ConversationStore = convoStore === undefined ? new MemoryStore() : convoStore;
this.use(conversationContext(store, this.logger));
}

// Should be last to avoid exposing partially initialized app
this.receiver.init(this);
}

/**
Expand Down Expand Up @@ -422,7 +422,7 @@ export default class App {
/**
* Handles events from the receiver
*/
private async onIncomingEvent(event: ReceiverEvent): Promise<void> {
public async processEvent(event: ReceiverEvent): Promise<void> {
const { body, ack } = event;
// TODO: when generating errors (such as in the say utility) it may become useful to capture the current context,
// or even all of the args, as properties of the error. This would give error handling code some ability to deal
Expand All @@ -440,13 +440,15 @@ export default class App {

// Initialize context (shallow copy to enforce object identity separation)
const source = buildSource(type, conversationId, bodyArg);
const authorizeResult = await (this.authorize(source, bodyArg).catch((error) => {
this.onGlobalError(authorizationErrorFromOriginal(error));
}));
if (authorizeResult === undefined) {
let authorizeResult;

try {
authorizeResult = await this.authorize(source, bodyArg);
} catch (error) {
this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.');
stevengill marked this conversation as resolved.
Show resolved Hide resolved
return;
return this.handleError(error);
}

const context: Context = { ...authorizeResult };

// Factory for say() utility
Expand Down Expand Up @@ -567,15 +569,15 @@ export default class App {
...(listenerArgs as MiddlewareContext<AnyMiddlewareArgs>),
});
} catch (error) {
this.onGlobalError(error);
return this.handleError(error);
}
}

/**
* Global error handler. The final destination for all errors (hopefully).
*/
private onGlobalError(error: Error): void {
this.errorHandler(asCodedError(error));
public handleError(error: Error): Promise<void> {
return this.errorHandler(asCodedError(error));
}

}
Expand Down Expand Up @@ -632,6 +634,8 @@ function isBlockActionOrInteractiveMessageBody(
function defaultErrorHandler(logger: Logger): ErrorHandler {
return (error) => {
stevengill marked this conversation as resolved.
Show resolved Hide resolved
logger.error(error);

return Promise.reject(error);
};
}

Expand Down Expand Up @@ -662,15 +666,3 @@ function selectToken(context: Context): string | undefined {

/* Instrumentation */
addAppMetadata({ name: packageJson.name, version: packageJson.version });

/* Error handling helpers */
function authorizationErrorFromOriginal(original: Error): AuthorizationError {
const error = errorWithCode('Authorization of incoming event did not succeed.', ErrorCode.AuthorizationError);
(error as AuthorizationError).original = original;
return error as AuthorizationError;
}

export interface AuthorizationError extends CodedError {
code: ErrorCode.AuthorizationError;
original: Error;
}
36 changes: 11 additions & 25 deletions src/ExpressReceiver.spec.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
// tslint:disable:no-implicit-dependencies
import 'mocha';

import { Logger, LogLevel } from '@slack/logger';
import { assert } from 'chai';
import { Request, Response } from 'express';
import { Agent } from 'http';
import sinon, { SinonFakeTimers } from 'sinon';
import { Readable } from 'stream';

import ExpressReceiver, {
respondToSslCheck,
respondToUrlVerification,
verifySignatureAndParseBody,
verifySignatureAndParseRawBody,
} from './ExpressReceiver';

describe('ExpressReceiver', () => {

const noopLogger: Logger = {
debug(..._msg: any[]): void { /* noop */ },
info(..._msg: any[]): void { /* noop */ },
Expand Down Expand Up @@ -43,10 +40,6 @@ describe('ExpressReceiver', () => {
signingSecret: 'my-secret',
logger: noopLogger,
endpoints: { events: '/custom-endpoint' },
agent: new Agent({
maxSockets: 999,
}),
clientTls: undefined,
});
assert.isNotNull(receiver);
});
Expand All @@ -58,6 +51,7 @@ describe('ExpressReceiver', () => {
signingSecret: 'my-secret',
logger: noopLogger,
});

await receiver.start(9999);
await receiver.stop();
});
Expand Down Expand Up @@ -102,7 +96,7 @@ describe('ExpressReceiver', () => {
});
});

describe('url_verification requset handler', () => {
describe('url_verification request handler', () => {
it('should handle valid requests', async () => {
// Arrange
// tslint:disable-next-line: no-object-literal-type-assertion
Expand Down Expand Up @@ -141,7 +135,7 @@ describe('ExpressReceiver', () => {
});
});

describe('verifySignatureAndParseBody', () => {
describe('verifySignatureAndParseRawBody', () => {

let clock: SinonFakeTimers;

Expand Down Expand Up @@ -196,7 +190,7 @@ describe('ExpressReceiver', () => {
const next = (error: any) => { state.error = error; };

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);
const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
await verifier(req, resp, next);
}

Expand Down Expand Up @@ -245,24 +239,15 @@ describe('ExpressReceiver', () => {
const result: any = {};
const resp = buildResponseToVerify(result);

let error: string = '';
let warn: string = '';
const logger = {
error: (msg: string) => { error = msg; },
warn: (msg: string) => { warn = msg; },
} as any as Logger;

const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(logger, signingSecret);
const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
await verifier(req, resp, next);

// Assert
assert.equal(result.code, 400);
assert.equal(result.sent, true);
assert.equal(error, 'Failed to parse body as JSON data for content-type: undefined');
aoberoi marked this conversation as resolved.
Show resolved Hide resolved
assert.equal(warn, 'Parsing request body failed (error: SyntaxError: Unexpected token o in JSON at position 1)');
}

it('should fail to parse request body without content-type header', async () => {
Expand Down Expand Up @@ -301,7 +286,7 @@ describe('ExpressReceiver', () => {
const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);
const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
await verifier(req, resp, next);

// Assert
Expand Down Expand Up @@ -355,7 +340,8 @@ describe('ExpressReceiver', () => {
const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);

const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
await verifier(req, resp, next);

// Assert
Expand Down Expand Up @@ -388,7 +374,7 @@ describe('ExpressReceiver', () => {
const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);
const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
await verifier(req, resp, next);

// Assert
Expand All @@ -414,7 +400,7 @@ describe('ExpressReceiver', () => {
const next = sinon.fake();

// Act
const verifier = verifySignatureAndParseBody(noopLogger, signingSecret);
const verifier = verifySignatureAndParseRawBody(noopLogger, signingSecret);
verifier(req, resp, next);
await verifier(req, resp, next);

Expand Down
Loading