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

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

## 1.2.4-beta.2 (Unreleased)

- Replaced the use of the 'express' module with a Node-native http server, shrinking the resulting identity module considerably
- `DefaultAzureCredential`'s implementation for browsers was simplified to throw a simple error instead of trying credentials that were already not supported for the browser.
- Breaking Change: `InteractiveBrowserCredential` for the browser now requires the client ID to be provided.
- Documentation was added to elaborate on how to configure an AAD application to support `InteractiveBrowserCredential`.
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,14 +86,14 @@
"@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",
"axios": "^0.21.1",
"events": "^3.0.0",
"express": "^4.16.3",
"jws": "^4.0.0",
"msal": "^1.0.2",
"open": "^7.0.0",
"qs": "^6.7.0",
"stoppable": "^1.1.0",
"tslib": "^2.0.0",
"uuid": "^8.3.0"
},
Expand Down
143 changes: 92 additions & 51 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.authorityHost) {
if (options.authorityHost.endsWith("/")) {
Expand Down Expand Up @@ -112,74 +116,111 @@ export class InteractiveBrowserCredential implements TokenCredential {
await open(response);
}

private async acquireTokenFromBrowser(scopeArray: string[]): Promise<AccessToken | null> {
// eslint-disable-next-line
return new Promise<AccessToken | null>(async (resolve, reject) => {
// eslint-disable-next-line
let listen: http.Server | undefined;
let socketToDestroy: Socket | undefined;
private acquireTokenFromBrowser(scopeArray: string[]): Promise<AccessToken | null> {
return new Promise<AccessToken | null>((resolve, reject) => {
const socketToDestroy: Socket[] = [];

function cleanup(): void {
if (listen) {
listen.close();
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;
}
if (socketToDestroy) {
socketToDestroy.destroy();
let url: URL;
try {
url = new URL(req.url, this.redirectUri);
} catch (e) {
reject(
new Error(
`Interactive Browser Authentication Error "Did not receive token with a valid expiration"`
Copy link
Member

Choose a reason for hiding this comment

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

The double quotes in the second part of the error message feel a little strange

)
);
return;
}
}

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

app.get("/", async (req, res) => {
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.push(socket));
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) {
this.openAuthCodeUrl(scopeArray).catch((e) => {
cleanup();
throw e;
reject(e);
});

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

for (const socket of socketToDestroy) {
socket.destroy();
}

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