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 10 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.

4 changes: 4 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@

## 1.2.4-beta.2 (Unreleased)

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

whoopsy, merge arrows got commited

- Replaced the use of the 'express' module with a Node-native http server, shrinking the resulting identity module considerably
=======
- Breaking change: `DefaultAzureCredential` now only uses `InteractiveBrowserCredential` in the browser since it's the only credential intended to be used to authenticate within browsers.
>>>>>>> 1827d69a042da834f813c9f78b8be0b3bd8aa6f1

## 1.2.4-beta.1 (2021-02-12)

Expand Down
4 changes: 2 additions & 2 deletions sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@
"@azure/msal-node": "1.0.0-beta.6",
"@azure/msal-browser": "2.9.0",
"@opentelemetry/api": "^0.10.2",
"@types/express": "^4.16.0",
"@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
120 changes: 77 additions & 43 deletions sdk/identity/identity/src/credentials/interactiveBrowserCredential.ts
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,71 +120,101 @@ 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();
const requestListener = (req: http.IncomingMessage, res: http.ServerResponse) => {
if (!req.url) {
reject(
new Error(
`Interactive Browser Authentication Error "Did not receive token with a valid expiration"`
)
);
return;
}
}

// Create Express App and Routes
const app = express();

app.get("/", async (req, res) => {
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.

I'd probably put a try/catch around everything before calling into acquireTokenByCode so you can reject with an Error if URL throws or something

const tokenRequest: AuthorizationCodeRequest = {
code: req.query.code as string,
code: url.searchParams.get("code")!,
redirectUri: this.redirectUri,
scopes: scopeArray
};

try {
const authResponse = await this.msalClient.acquireTokenByCode(tokenRequest);
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);
logger.getToken.info(formatSuccess(scopeArray));

resolve({
expiresOnTimestamp,
token: authResponse.accessToken
});
} else {
this.msalClient
.acquireTokenByCode(tokenRequest)
.then((authResponse) => {
const successMessage = `Authentication Complete. You can close the browser and return to the application.`;
if (authResponse && authResponse.expiresOn) {
const expiresOnTimestamp = authResponse?.expiresOn.valueOf();
res.writeHead(200);
res.end(successMessage);
logger.getToken.info(formatSuccess(scopeArray));

resolve({
expiresOnTimestamp,
token: authResponse.accessToken
});
} else {
const errorMessage = formatError(
scopeArray,
`${url.searchParams.get("error")}. ${url.searchParams.get("error_description")}`
);
res.writeHead(500);
res.end(errorMessage);
logger.getToken.info(errorMessage);

reject(
new Error(
`Interactive Browser Authentication Error "Did not receive token with a valid expiration"`
)
);
}
cleanup();
return;
Copy link
Member

Choose a reason for hiding this comment

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

this return isn't needed / doesn't do anything

})
.catch(() => {
const errorMessage = formatError(
scopeArray,
`${url.searchParams.get("error")}. ${url.searchParams.get("error_description")}`
);
res.writeHead(500);
res.end(errorMessage);
logger.getToken.info(errorMessage);

reject(
new Error(
`Interactive Browser Authentication Error "Did not receive token with a valid expiration"`
)
);
}
} catch (error) {
const errorMessage = formatError(
scopeArray,
`${req.query["error"]}. ${req.query["error_description"]}`
);
res.status(500).send(errorMessage);
logger.getToken.info(errorMessage);
reject(new Error(errorMessage));
} finally {
cleanup();
}
});
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();
}
}
});
}
}