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] Improvements to MSAL support #11407

Merged
merged 12 commits into from
Sep 29, 2020
Merged
Show file tree
Hide file tree
Changes from 6 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: 9 additions & 8 deletions 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: 2 additions & 2 deletions sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,14 @@
"@azure/core-http": "^1.1.6",
"@azure/core-tracing": "1.0.0-preview.9",
"@azure/logger": "^1.0.0",
"@azure/msal-node": "^1.0.0-alpha.5",
"@azure/msal-node": "^1.0.0-alpha.6",
Copy link
Member

Choose a reason for hiding this comment

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

should we pin this? or are they not making breaking changes? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're still making changes, yeah.

"@azure/msal-common": "^1.3.0",
"@opentelemetry/api": "^0.10.2",
"events": "^3.0.0",
"jws": "^4.0.0",
"msal": "^1.0.2",
"open": "^7.0.0",
"qs": "^6.7.0",
"axios": "^0.19.2",
"tslib": "^2.0.0",
"uuid": "^8.1.0"
},
Expand Down
8 changes: 8 additions & 0 deletions sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ export class InteractiveBrowserCredential implements TokenCredential {

// @public
export interface InteractiveBrowserCredentialOptions extends TokenCredentialOptions {
// Warning: (ae-forgotten-export) The symbol "AuthenticationRecord" needs to be exported by the entry point index.d.ts
sophiajt marked this conversation as resolved.
Show resolved Hide resolved
authenticationRecord?: AuthenticationRecord;
cacheOptions?: {
cachePlugin?: {
readFromStorage: () => Promise<string>;
writeToStorage: (getMergedState: (oldState: string) => string) => Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

feels kinda bad that write isn't the same shape as read... what does getMergedState() do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from having to merge our changes into the cache rather than just writing out (other applications and/or other languages may have written to the shared cache)

Copy link
Member

Choose a reason for hiding this comment

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

Right, so you always hand back oldState being the last string you read in with readFromStorage and then write that? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, the user won't even fulfill this contract. We'll soon be providing helper functions that will create the storage/persistence plugin for you.

I'm tempted to chalk this up as some of the underlying implementation leaking through a little bit, though I'm not sure how to resolve as we still currently allow users to implement their own persistence plugins.

@schaabs - any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

We've been back and forth on whether we'll be adding APIs to allow people to write the cache themselves. If we can hide these for now it would be ideal. But given it's a preview if we can't easily hide these in the short term we could include them, but we should be clear it's subject to change.

};
};
clientId?: string;
loginStyle?: BrowserLoginStyle;
postLogoutRedirectUri?: string | (() => string);
Expand Down
52 changes: 50 additions & 2 deletions sdk/identity/identity/src/client/identityClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,14 @@ import {
RequestPrepareOptions,
GetTokenOptions,
createPipelineFromOptions,
isNode
isNode,
OperationArguments,
OperationSpec,
RawHttpHeaders,
HttpHeaders
} from "@azure/core-http";
import { INetworkModule, NetworkRequestOptions, NetworkResponse } from "@azure/msal-common";

import { CanonicalCode } from "@opentelemetry/api";
import { AuthenticationError, AuthenticationErrorName } from "./errors";
import { createSpan } from "../util/tracing";
Expand All @@ -36,7 +42,7 @@ export interface TokenResponse {
refreshToken?: string;
}

export class IdentityClient extends ServiceClient {
export class IdentityClient extends ServiceClient implements INetworkModule {
public authorityHost: string;

constructor(options?: TokenCredentialOptions) {
Expand Down Expand Up @@ -184,6 +190,48 @@ export class IdentityClient extends ServiceClient {
}
}

sendGetRequestAsync<T>(
Copy link
Member

Choose a reason for hiding this comment

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

why isn't this an async function so we can use await inside instead?

url: string,
options?: NetworkRequestOptions
): Promise<NetworkResponse<T>> {
const webResource = new WebResource(url, "GET", options?.body, {}, options?.headers);

return this.sendRequest(webResource).then((x) => {
sophiajt marked this conversation as resolved.
Show resolved Hide resolved
let headers: Record<string, string> = {};
let kv = x.headers.headersArray();
for (let idx in kv) {
headers[kv[idx].name] = kv[idx].value;
}
sophiajt marked this conversation as resolved.
Show resolved Hide resolved

return {
body: x.parsedBody as T,
headers,
status: x.status
};
});
}
Copy link
Contributor

@sadasant sadasant Sep 24, 2020

Choose a reason for hiding this comment

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

Sorry to ask, but I'd like to know: Don't we need tracing here?


sendPostRequestAsync<T>(
Copy link
Member

Choose a reason for hiding this comment

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

this can also be an async function. The Async suffix on these methods feels very .NET though, we don't follow that convention anywhere else.

url: string,
options?: NetworkRequestOptions
): Promise<NetworkResponse<T>> {
const webResource = new WebResource(url, "POST", options?.body, {}, options?.headers);

return this.sendRequest(webResource).then((x) => {
let headers: Record<string, string> = {};
let kv = x.headers.headersArray();
for (let idx in kv) {
headers[kv[idx].name] = kv[idx].value;
}
sophiajt marked this conversation as resolved.
Show resolved Hide resolved

return {
body: x.parsedBody as T,
headers,
status: x.status
};
});
}

static getDefaultOptions(): TokenCredentialOptions {
return {
authorityHost: DefaultAuthorityHost
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,26 @@ import { TokenCredentialOptions, IdentityClient } from "../client/identityClient
import { DefaultTenantId, DeveloperSignOnClientId } from "../constants";
import { Socket } from "net";

const SERVER_PORT = process.env.PORT || 80;

import express from "express";
import { PublicClientApplication, TokenCache, AuthorizationCodeRequest } from "@azure/msal-node";
import {
PublicClientApplication,
TokenCache,
AuthorizationCodeRequest,
Configuration
} from "@azure/msal-node";
import open from "open";
import path from "path";
import http from "http";
import { CredentialUnavailable } from "../client/errors";

const BrowserNotSupportedError = new Error(
"InteractiveBrowserCredential is not supported in Node.js."
);
const logger = credentialLogger("InteractiveBrowserCredential");

class AuthenticationRequired extends CredentialUnavailable {
constructor(message?: string) {
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 have to worry about setting this.name to be AuthenticationRequired?

super(message);
}
}

interface AuthenticationRecord {
authority?: string;
homeAccountId: string;
Expand All @@ -46,15 +53,16 @@ export class InteractiveBrowserCredential implements TokenCredential {
private redirectUri: string;
private authorityHost: string;
private authenticationRecord: AuthenticationRecord | undefined;
private port: number;

constructor(options?: InteractiveBrowserCredentialOptions) {
this.identityClient = new IdentityClient(options);
this.tenantId = (options && options.tenantId) || DefaultTenantId;
this.clientId = (options && options.clientId) || DeveloperSignOnClientId;

// Future update: this is for persistent caching
this.persistenceEnabled = false;
this.authenticationRecord = undefined;
this.persistenceEnabled = this.persistenceEnabled = options?.cacheOptions !== undefined;
this.authenticationRecord = options?.authenticationRecord;

if (options && options.redirectUri) {
if (typeof options.redirectUri === "string") {
Expand All @@ -66,6 +74,12 @@ export class InteractiveBrowserCredential implements TokenCredential {
this.redirectUri = "http://localhost";
}

const url = new 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 believe this won't work on older Nodes. You can see how I do this in core-https: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-https/src/util/url.ts

this.port = parseInt(url.port);
if (this.port == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

parseInt can return NaN in failure modes, but it isn't == 0

this.port = 80;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the odd scenario where somebody has the processor when this method is called, could it be possible for something actively reading this property to read it in a NaN state?

If that isn't plausible, then ignore this! 🌞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you're asking here. Though maybe this will help clarify the above, from parseInt docs:

Return value
An integer parsed from the given string.

Or NaN when

the radix is smaller than 2 or bigger than 36, or
the first non-whitespace character cannot be converted to a number.

There may be clearer ways to do the above. I'm just detecting the NaN case and defaulting the port.


if (options && options.authorityHost) {
if (options.authorityHost.endsWith("/")) {
this.authorityHost = options.authorityHost + this.tenantId;
Expand All @@ -76,13 +90,13 @@ export class InteractiveBrowserCredential implements TokenCredential {
this.authorityHost = "https://login.microsoftonline.com/" + this.tenantId;
}

const publicClientConfig = {
const publicClientConfig: Configuration = {
auth: {
clientId: this.clientId,
authority: this.authorityHost,
redirectUri: this.redirectUri
authority: this.authorityHost
},
cache: undefined
cache: options?.cacheOptions,
system: { networkClient: this.identityClient }
};
this.pca = new PublicClientApplication(publicClientConfig);
this.msalCacheManager = this.pca.getTokenCache();
Expand All @@ -104,7 +118,41 @@ export class InteractiveBrowserCredential implements TokenCredential {
): Promise<AccessToken | null> {
const scopeArray = typeof scopes === "object" ? scopes : [scopes];

return this.acquireTokenFromBrowser(scopeArray);
if (this.authenticationRecord && this.persistenceEnabled) {
try {
return this.acquireTokenFromCache();
} catch (e) {
if (e instanceof AuthenticationRequired) {
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit brittle, since errors from different constructors/contexts are not instanceof, hence a name property check is sometimes better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a link to a preferred pattern, by chance?

Copy link
Member

Choose a reason for hiding this comment

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

If you set this.name = "YourErrorType" in your custom error constructor, you can check if e.name === "YourErrorType". It's not necessarily better, but duck typing on a discriminant seems a little better than relying on two packages using the same constructor function for an error.

Copy link
Member

Choose a reason for hiding this comment

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

It'll also print your custom error's class name when toString is called, along with message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily, we have full control here, since we both define and are the only ones throwing this error. I think I'll leave it as-is for now, but something we might want to throw into the next JS/TS team discussion around preferred coding patterns.

return this.acquireTokenFromBrowser(scopeArray);
} else {
throw e;
}
}
} else {
return this.acquireTokenFromBrowser(scopeArray);
}
}

private async acquireTokenFromCache(): Promise<AccessToken | null> {
await this.msalCacheManager.readFromPersistence();
// const contents = this.msalCacheManager.serialize();
// const accounts = this.msalCacheManager.getAllAccounts();
sophiajt marked this conversation as resolved.
Show resolved Hide resolved

const silentRequest = {
account: this.authenticationRecord!,
scopes: ["https://vault.azure.net/user_impersonation", "https://vault.azure.net/.default"]
};

try {
const response = await this.pca.acquireTokenSilent(silentRequest);
logger.info(`Successful silent token acquisition:\nResponse: \n${response}`);
sophiajt marked this conversation as resolved.
Show resolved Hide resolved
return {
expiresOnTimestamp: response.expiresOn.getTime(),
token: response.accessToken
};
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that handling ALL exceptions thrown from acquireTokenSilent is the right thing to do here. Does MSAL document an particular error they will raise in the case that interaction is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the MSAL docs:

"In case the refresh_token is expired or not found, an error is thrown and the guidance is for the user to call any interactive token acquisition API (eg: acquireTokenByCode())."

We could ask them for more details, and see if they're comfortable locking down the exception side of the API or exposing more information.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should follow up with them on this. It would be nice to make the error contract for these cases a bit tighter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like it's out of scope for the time being. But they can look into it for a follow-up. I think we can take their change and modify what we do. It doesn't feel like necessarily a breaking change for us.

throw new AuthenticationRequired("Could not authenticate silently using the cache");
}
}

private async openAuthCodeUrl(scopeArray: string[]): Promise<void> {
Expand All @@ -115,6 +163,14 @@ export class InteractiveBrowserCredential implements TokenCredential {

const response = await this.pca.getAuthCodeUrl(authCodeUrlParameters);
await open(response);

if (this.persistenceEnabled) {
try {
await this.msalCacheManager.readFromPersistence();
} catch (e) {
throw e;
}
sophiajt marked this conversation as resolved.
Show resolved Hide resolved
}
}

private async acquireTokenFromBrowser(scopeArray: string[]): Promise<AccessToken | null> {
Expand Down Expand Up @@ -146,7 +202,7 @@ export class InteractiveBrowserCredential implements TokenCredential {
try {
const authResponse = await this.pca.acquireTokenByCode(tokenRequest);
res.sendStatus(200);
logger.info(`authResponse: ${authResponse}`);

if (this.persistenceEnabled) {
this.msalCacheManager.writeToPersistence();
}
Expand All @@ -168,8 +224,8 @@ export class InteractiveBrowserCredential implements TokenCredential {
}
});

listen = app.listen(SERVER_PORT, () =>
logger.info(`Msal Node Auth Code Sample app listening on port ${SERVER_PORT}!`)
listen = app.listen(this.port, () =>
logger.info(`Msal Node Auth Code Sample app listening on port ${this.port}!`)
);
listen.on("connection", (socket) => (socketToDestroy = socket));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ import { TokenCredentialOptions } from "../client/identityClient";
*/
export type BrowserLoginStyle = "redirect" | "popup";

/**
* The record to use to find the cached tokens in the cache
*/
export interface AuthenticationRecord {
authority?: string;
homeAccountId: string;
environment: string;
tenantId: string;
username: string;
}

/**
* Defines options for the InteractiveBrowserCredential class.
*/
Expand Down Expand Up @@ -46,4 +57,19 @@ export interface InteractiveBrowserCredentialOptions extends TokenCredentialOpti
* The client (application) ID of an App Registration in the tenant.
*/
clientId?: string;

/**
* The cache options to use when credentials are being checked.
*/
cacheOptions?: {
cachePlugin?: {
readFromStorage: () => Promise<string>;
writeToStorage: (getMergedState: (oldState: string) => string) => Promise<void>;
};
};

/**
* The authentication record to use to find existing tokens in the cache
*/
authenticationRecord?: AuthenticationRecord;
}