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

[Identity] Remove express #13800

Merged
merged 14 commits into from
Feb 24, 2021
Merged
Show file tree
Hide file tree
Changes from 5 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
17 changes: 16 additions & 1 deletion common/config/rush/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,10 @@
"@azure/msal-browser": "2.9.0",
"@opentelemetry/api": "^0.10.2",
"@types/express": "^4.16.0",
Copy link
Member

Choose a reason for hiding this comment

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

can get rid of express types too right?

"@types/stoppable": "^1.1.0",
"stoppable": "^1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

plz alphabetize deps

"axios": "^0.21.1",
"events": "^3.0.0",
"express": "^4.16.3",
"jws": "^4.0.0",
"msal": "^1.0.2",
"open": "^7.0.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { Socket } from "net";
import { AuthenticationRequired, MsalClient } from "../client/msalClient";
import { AuthorizationCodeRequest } from "@azure/msal-node";

import express from "express";
import open from "open";
import http from "http";
import stoppable from "stoppable";

import { checkTenantId } from "../util/checkTenantId";

const logger = credentialLogger("InteractiveBrowserCredential");
Expand All @@ -26,6 +27,7 @@ const logger = credentialLogger("InteractiveBrowserCredential");
export class InteractiveBrowserCredential implements TokenCredential {
private redirectUri: string;
private port: number;
private hostname: string;
private msalClient: MsalClient;

constructor(options?: InteractiveBrowserCredentialOptions) {
Expand Down Expand Up @@ -53,6 +55,8 @@ export class InteractiveBrowserCredential implements TokenCredential {
this.port = 80;
}

this.hostname = url.hostname;

let authorityHost;
if (options && options.authorityHost) {
if (options.authorityHost.endsWith("/")) {
Expand Down Expand Up @@ -116,24 +120,14 @@ export class InteractiveBrowserCredential implements TokenCredential {
// eslint-disable-next-line
return new Promise<AccessToken | null>(async (resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

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

Because this method just returns the promise constructor, acquireTokenFromBrowser doesn't need to be async, it can just return a Promise. If you remove that, I bet you can get rid of the eslint disable too!

// eslint-disable-next-line
let listen: http.Server | undefined;
let socketToDestroy: Socket | undefined;

function cleanup(): void {
if (listen) {
listen.close();
}
if (socketToDestroy) {
socketToDestroy.destroy();
}
}

// Create Express App and Routes
Copy link
Member

Choose a reason for hiding this comment

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

nit: comment can use an update

const app = express();

app.get("/", async (req, res) => {
const requestListener = async (req: http.IncomingMessage, res: http.ServerResponse) => {
const url = new URL(req.url!, this.redirectUri);
Copy link
Member

Choose a reason for hiding this comment

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

do we ever have to be worried about req.url being undefined? Are the typings just bad? What would happen in the failure case, URL constructor throwing?

Actually what even happens if requestListener fails? Since it's an async function it will reject, does Node do something with that rejection? Since all that seems to happen is the function gets registered on the request event, I don't think it would, so Node would hit an "unhandled rejection" error and possibly die.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like handling for async functions as event handlers (and actually subscribing to their rejection) is still very experimental: https://nodejs.org/api/events.html#events_capture_rejections_of_promises

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xirzec - can you double check the new approach before I land it?

Copy link
Member

Choose a reason for hiding this comment

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

Taking a look now!

const tokenRequest: AuthorizationCodeRequest = {
code: req.query.code as string,
code: url.searchParams.get("code")!,
redirectUri: this.redirectUri,
scopes: scopeArray
};
Expand All @@ -143,7 +137,8 @@ export class InteractiveBrowserCredential implements TokenCredential {
const successMessage = `Authentication Complete. You can close the browser and return to the application.`;
if (authResponse && authResponse.expiresOn) {
const expiresOnTimestamp = authResponse?.expiresOn.valueOf();
res.status(200).send(successMessage);
res.writeHead(200);
res.end(successMessage);
logger.getToken.info(formatSuccess(scopeArray));

resolve({
Expand All @@ -160,27 +155,44 @@ export class InteractiveBrowserCredential implements TokenCredential {
} catch (error) {
const errorMessage = formatError(
scopeArray,
`${req.query["error"]}. ${req.query["error_description"]}`
`${url.searchParams.get("error")}. ${url.searchParams.get("error_description")}`
);
res.status(500).send(errorMessage);
res.writeHead(500);
res.end(errorMessage);
logger.getToken.info(errorMessage);
reject(new Error(errorMessage));
} finally {
cleanup();
}
});
};
const app = http.createServer(requestListener);

listen = app.listen(this.port, () =>
const listen = app.listen(this.port, this.hostname, () =>
logger.info(`InteractiveBrowerCredential listening on port ${this.port}!`)
);
listen.on("connection", (socket) => (socketToDestroy = socket));
app.on("connection", (socket) => (socketToDestroy = socket));
Copy link
Member

Choose a reason for hiding this comment

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

this fires per connection, right? If someone refreshes their browser or something, could you end up calling it twice and losing the first socket when socketToDestroy gets overwritten?

const server = stoppable(app);
Copy link
Member

Choose a reason for hiding this comment

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

I was curious about why this is needed - what connections are preventing app from stopping gracefully?

If we do want to use this, should we either force-close with a grace of 0 or allow some grace period?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maorleger - there's just a long delay on when the server shuts down, afaict. I read through forums on the issue and this package was suggested as a good fix if you want to use the builtin http server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any restrictions on adding third party libraries? I keep forgetting. @bterlson

Copy link
Member

Choose a reason for hiding this comment

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

They need to be approved, yes.

@jonathandturner can you say more about the delay you're solving here? Is it because the inbound http connection uses keepalive or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly what causes it but I did notice something similar when we used express. We had to manually delete the express listener for it to shutdown, otherwise it stayed open. For the built-in http sever, this happens even if you manually close it, which is why they made this additional library. The comments on some of the threads are kind of interesting. No one is completely sure why it happens to work this way and generally people just get pointed to this library as the fix.

Copy link
Member

Choose a reason for hiding this comment

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

At a quick glance, this dep seems very small/targeted, so it doesn't bother me, though I am curious to learn more about why Node never bothered to fix this natively

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the additional details @jonathandturner. This new dependency seems fine from my perspective.


try {
await this.openAuthCodeUrl(scopeArray);
} catch (e) {
cleanup();
throw e;
}
Copy link
Member

Choose a reason for hiding this comment

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

It's not super clear to me if this will do the right thing, since we're in a promise constructor asynchronously. I think it'd be safer to rewrite this bit and ditch the async from the Promise constructor callback:

Suggested change
try {
await this.openAuthCodeUrl(scopeArray);
} catch (e) {
cleanup();
throw e;
}
this.openAuthCodeUrl(scopeArray).catch(e => {
cleanup();
reject(e);
});


function cleanup(): void {
if (listen) {
listen.close();
}
if (socketToDestroy) {
socketToDestroy.destroy();
}

if (server) {
server.close();
server.stop();
}
}
});
}
}