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

[Streaming, Preview] rewrite useWebSocket for low-level usage, remove from processActivity #1433

Merged
merged 6 commits into from
Dec 2, 2019
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
48 changes: 28 additions & 20 deletions libraries/botbuilder/src/botFrameworkAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
* Licensed under the MIT License.
*/

import { IncomingMessage } from 'http';
import { IncomingMessage, STATUS_CODES } from 'http';
import { Socket } from 'net';
import * as os from 'os';

import { Activity, ActivityTypes, BotAdapter, BotCallbackHandlerKey, ChannelAccount, ConversationAccount, ConversationParameters, ConversationReference, ConversationsResult, IUserTokenProvider, ResourceResponse, TokenResponse, TurnContext } from 'botbuilder-core';
Expand Down Expand Up @@ -755,9 +756,6 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide
* ```
*/
public async processActivity(req: WebRequest, res: WebResponse, logic: (context: TurnContext) => Promise<any>): Promise<void> {
if (this.settings.enableWebSockets && req.method === GET && (req.headers.Upgrade || req.headers.upgrade)) {
return this.useWebSocket(req, res, logic);
}

let body: any;
let status: number;
Expand Down Expand Up @@ -1151,36 +1149,37 @@ export class BotFrameworkAdapter extends BotAdapter implements IUserTokenProvide
* @param res The response sent on error or connection termination.
* @param logic The logic that will handle incoming requests.
*/
public async useWebSocket(req: WebRequest, res: WebResponse, logic: (context: TurnContext) => Promise<any>): Promise<void> {
if (!logic) {
throw new Error('Streaming logic needs to be provided to `useWebSocket`');
}

public async useWebSocket(req: IncomingMessage, socket: Socket, head: Buffer, logic: (context: TurnContext) => Promise<any>): Promise<void> {
if (!this.webSocketFactory || !this.webSocketFactory.createWebSocket) {
throw new Error('BotFrameworkAdapter must have a WebSocketFactory in order to support streaming.');
}

this.logic = logic;

// Restify-specific check.
if (typeof((res as any).claimUpgrade) !== 'function') {
throw new Error('ClaimUpgrade is required for creating WebSocket connection.');
if (!logic) {
throw new Error('Streaming logic needs to be provided to `useWebSocket`');
}

this.logic = logic;

try {
await this.authenticateConnection(req, this.settings.channelService);
} catch (err) {
// Set the correct status code for the socket to send back to the channel.
res.status(StatusCodes.UNAUTHORIZED);
res.send(err.message);
// If the authenticateConnection call fails, send back the correct error code and close
// the connection.
if (typeof(err.message) === 'string' && err.message.toLowerCase().startsWith('unauthorized')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

string matching is error prone (P1) and expensive (P2)

One way to improve confidence is to use a string constant that's shared between the component throwing the error and this component, that way if the message changes, we're not exposed.

Ideally, we'd move to a status code based system which we can avoid doing message parsing in.

abortWebSocketUpgrade(socket, 401);
} else if (typeof(err.message) === 'string' && err.message.toLowerCase().startsWith(`'authheader'`)) {
abortWebSocketUpgrade(socket, 400);
} else {
abortWebSocketUpgrade(socket, 500);
Copy link
Member

Choose a reason for hiding this comment

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

Why dont we send the error message in the response? Seems impossible to detect the error without some more info.

Copy link
Member

Choose a reason for hiding this comment

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

Or are they usually cryptic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that this isn't ideal. Maybe we can use NODE_ENV to detect if running in prod or not?

Also, this is a great example where proper o11y (logging/tracing) would be helpful

}
Comment on lines +1156 to +1164
Copy link
Member Author

Choose a reason for hiding this comment

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

@carlosscastro I'm not happy with this code, but I'm not sure if there are better steps that can be taken here.

Node.js does not have HTTP Status Code-specific errors, so our underlying auth layers throw new Error with different message strings. Due to this, the code parses the message to attempt to determine the correct status code to send back before destroying the socket connection.

I have a test that covers the 401 Unauthorized route, and tested the other two routes manually.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that hurts, but I understand. Can we improve the error reporting in the auth layers instead of here? Maybe we could create an abstraction that processes error messages and creates well defined errors + codes, and then the auth layer uses that in order to throw richer errors. Sounds like something that may not be possible without breaking backward compat though.

If it is not possible to move this logic to the auth layer, then I guess we are out of choices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #1445 for this. I've tentatively labeled it as a P2 for R7. Since this is internal, I don't want to block on this specific code.

I'd like to get this in so that I can have the package available for the WebChat DLS bug bash tomorrow.


// Re-throw the error so the developer will know what occurred.
throw err;
}

const upgrade = (res as any).claimUpgrade();
const socket = await this.webSocketFactory.createWebSocket(req as IncomingMessage, upgrade.socket, upgrade.head);
const nodeWebSocket = await this.webSocketFactory.createWebSocket(req, socket, head);

await this.startWebSocket(socket);
await this.startWebSocket(nodeWebSocket);
}

private async authenticateConnection(req: WebRequest, channelService?: string): Promise<void> {
Expand Down Expand Up @@ -1303,4 +1302,13 @@ function delay(timeout: number): Promise<void> {
return new Promise((resolve) => {
setTimeout(resolve, timeout);
});
}

function abortWebSocketUpgrade(socket: Socket, code: number) {
if (socket.writable) {
const connectionHeader = `Connection: 'close'\r\n`;
socket.write(`HTTP/1.1 ${code} ${STATUS_CODES[code]}\r\n${connectionHeader}\r\n`);
Copy link
Member

Choose a reason for hiding this comment

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

Just checking - could we have any cross platform issues with the \r\n?

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 should work on the different platforms, but I can check on a different one.

Copy link
Member

Choose a reason for hiding this comment

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

What about HTTP/2 calls to upgrade to WebSockets? Can we dynamically populate the HTTP version in this response? I'm not sure it matters, but it may lead to confusing debugging and AFAIK we don't specifically NOT support HTTP/2 anywhere. (The ABS channels are all HTTP/1.1, but there's nothing stopping an outside channel from using HTTP/2.) Not a ship stopper, but food for thought.

Copy link
Member

Choose a reason for hiding this comment

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

Also it looks like we're never sending the actual error message back now, is that right? That will make debugging problematic, especially if the code is a 500.

Copy link
Member Author

@stevengum stevengum Dec 2, 2019

Choose a reason for hiding this comment

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

}

socket.destroy();
}
Loading