-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from all commits
719e143
7b3136f
b822b66
b96f61b
47d4390
ff652ab
d8bd361
c25ade8
8aec185
f351060
e3af557
05eeb4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,15 @@ export class AuthenticationError extends Error { | |
// @public | ||
export const AuthenticationErrorName = "AuthenticationError"; | ||
|
||
// @public | ||
export interface AuthenticationRecord { | ||
authority?: string; | ||
environment: string; | ||
homeAccountId: string; | ||
tenantId: string; | ||
username: string; | ||
} | ||
|
||
// @public | ||
export class AuthorizationCodeCredential implements TokenCredential { | ||
constructor(tenantId: string | "common", clientId: string, clientSecret: string, authorizationCode: string, redirectUri: string, options?: TokenCredentialOptions); | ||
|
@@ -139,6 +148,13 @@ export class InteractiveBrowserCredential implements TokenCredential { | |
|
||
// @public | ||
export interface InteractiveBrowserCredentialOptions extends TokenCredentialOptions { | ||
authenticationRecord?: AuthenticationRecord; | ||
cacheOptions?: { | ||
cachePlugin?: { | ||
readFromStorage: () => Promise<string>; | ||
writeToStorage: (getMergedState: (oldState: string) => string) => Promise<void>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, so you always hand back There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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-node"; | ||
|
||
import { CanonicalCode } from "@opentelemetry/api"; | ||
import { AuthenticationError, AuthenticationErrorName } from "./errors"; | ||
import { createSpan } from "../util/tracing"; | ||
|
@@ -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) { | ||
|
@@ -184,6 +190,36 @@ export class IdentityClient extends ServiceClient { | |
} | ||
} | ||
|
||
sendGetRequestAsync<T>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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((response) => { | ||
return { | ||
body: response.parsedBody as T, | ||
headers: response.headers.rawHeaders(), | ||
status: response.status | ||
}; | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can also be an async function. The |
||
url: string, | ||
options?: NetworkRequestOptions | ||
): Promise<NetworkResponse<T>> { | ||
const webResource = new WebResource(url, "POST", options?.body, {}, options?.headers); | ||
|
||
return this.sendRequest(webResource).then((response) => { | ||
return { | ||
body: response.parsedBody as T, | ||
headers: response.headers.rawHeaders(), | ||
status: response.status | ||
}; | ||
}); | ||
} | ||
|
||
static getDefaultOptions(): TokenCredentialOptions { | ||
return { | ||
authorityHost: DefaultAuthorityHost | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,32 +4,30 @@ | |
/* eslint-disable @typescript-eslint/no-unused-vars */ | ||
|
||
import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http"; | ||
import { InteractiveBrowserCredentialOptions } from "./interactiveBrowserCredentialOptions"; | ||
import { | ||
InteractiveBrowserCredentialOptions, | ||
AuthenticationRecord | ||
} from "./interactiveBrowserCredentialOptions"; | ||
import { credentialLogger, formatError } from "../util/logging"; | ||
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"); | ||
|
||
interface AuthenticationRecord { | ||
authority?: string; | ||
homeAccountId: string; | ||
environment: string; | ||
tenantId: string; | ||
username: string; | ||
} | ||
class AuthenticationRequired extends CredentialUnavailable {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be worth setting a custom name on the error class. It also feels a little bad to not have the suffix Oh and if you're going to make a custom error type, shouldn't you export it somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can see how we set the prototype and name in RestError here: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-http/src/restError.ts |
||
|
||
/** | ||
* Enables authentication to Azure Active Directory inside of the web browser | ||
|
@@ -46,15 +44,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") { | ||
|
@@ -66,6 +65,12 @@ export class InteractiveBrowserCredential implements TokenCredential { | |
this.redirectUri = "http://localhost"; | ||
} | ||
|
||
const url = new URL(this.redirectUri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
this.port = parseInt(url.port); | ||
if (isNaN(this.port)) { | ||
this.port = 80; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! 🌞 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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; | ||
|
@@ -76,13 +81,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(); | ||
|
@@ -104,7 +109,37 @@ export class InteractiveBrowserCredential implements TokenCredential { | |
): Promise<AccessToken | null> { | ||
const scopeArray = typeof scopes === "object" ? scopes : [scopes]; | ||
|
||
return this.acquireTokenFromBrowser(scopeArray); | ||
if (this.authenticationRecord && this.persistenceEnabled) { | ||
return this.acquireTokenFromCache().catch((e) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure why this method isn't async so you can try/catch here instead of using |
||
if (e instanceof AuthenticationRequired) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a link to a preferred pattern, by chance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you set There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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"); | ||
return { | ||
expiresOnTimestamp: response.expiresOn.getTime(), | ||
token: response.accessToken | ||
}; | ||
} catch (e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that handling ALL exceptions thrown from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||
|
@@ -115,6 +150,10 @@ export class InteractiveBrowserCredential implements TokenCredential { | |
|
||
const response = await this.pca.getAuthCodeUrl(authCodeUrlParameters); | ||
await open(response); | ||
|
||
if (this.persistenceEnabled) { | ||
await this.msalCacheManager.readFromPersistence(); | ||
} | ||
} | ||
|
||
private async acquireTokenFromBrowser(scopeArray: string[]): Promise<AccessToken | null> { | ||
|
@@ -146,7 +185,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(); | ||
} | ||
|
@@ -168,8 +207,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)); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthenticationRecord
should have aclientId
property as well as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It comes from https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-common/src/account/AccountInfo.ts#L14-L20
Should we take in
clientId
and repackage it for MSAL as one of their fields?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data should be returned in the response from
acquireToken***
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take that back. In .NET we take that data into the constructor of the
InteractiveBrowserCredential
and store it as a property,ClientId
, on the instance:https://github.com/Azure/azure-sdk-for-net/blob/a8a0e8cdf5528017fa3dfd22cae81a879e7f0d39/sdk/identity/Azure.Identity/src/InteractiveBrowserCredential.cs#L75-L77
Then when we interactively authenticate we update the
AuthenticationRecord
also passing in the storedClientId
:https://github.com/Azure/azure-sdk-for-net/blob/a8a0e8cdf5528017fa3dfd22cae81a879e7f0d39/sdk/identity/Azure.Identity/src/InteractiveBrowserCredential.cs#L207-L214
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure I follow. Are you saying you do repack the value for MSAL and rename the field? Or just that .NET works differently than msal-node