-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] DeviceCodeCredential uses MSALClient #29405
Changes from 9 commits
a127b3a
3fba79b
a0066e3
6015489
9880823
ec347c8
c318ba0
3894946
068dbf8
5697e9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,26 +13,44 @@ import { | |
getKnownAuthorities, | ||
getMSALLogLevel, | ||
handleMsalError, | ||
msalToPublic, | ||
publicToMsal, | ||
} from "../utils"; | ||
|
||
import { AuthenticationRequiredError } from "../../errors"; | ||
import { CertificateParts } from "../types"; | ||
import { AuthenticationRecord, CertificateParts } from "../types"; | ||
import { IdentityClient } from "../../client/identityClient"; | ||
import { MsalNodeOptions } from "./msalNodeCommon"; | ||
import { calculateRegionalAuthority } from "../../regionalAuthority"; | ||
import { getLogLevel } from "@azure/logger"; | ||
import { resolveTenantId } from "../../util/tenantIdUtils"; | ||
import { DeviceCodePromptCallback } from "../../credentials/deviceCodeCredentialOptions"; | ||
|
||
/** | ||
* The logger for all MsalClient instances. | ||
*/ | ||
const msalLogger = credentialLogger("MsalClient"); | ||
|
||
export interface GetTokenWithSilentAuthOptions extends GetTokenOptions { | ||
/** | ||
* Disables automatic authentication. If set to true, the method will throw an error if the user needs to authenticate. | ||
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. should this be called 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. Heya @xirzec - Karishma and I chatted offline and we think that as backwards as this name is we will keep it throughout the codebase for consistency. At least it matches the meaning and behavior of the public API for better or worse. If you feel strongly, happy to make a follow-up PR with some renames! But merging for now |
||
* | ||
* @remarks | ||
* | ||
* This option will be set to `false` when the user calls `authenticate` directly on a credential that supports it. | ||
*/ | ||
disableAutomaticAuthentication?: boolean; | ||
} | ||
|
||
/** | ||
* Represents a client for interacting with the Microsoft Authentication Library (MSAL). | ||
*/ | ||
export interface MsalClient { | ||
getTokenByDeviceCode( | ||
arrayScopes: string[], | ||
userPromptCallback: DeviceCodePromptCallback, | ||
options?: GetTokenWithSilentAuthOptions, | ||
): Promise<AccessToken>; | ||
/** | ||
* Retrieves an access token by using a client certificate. | ||
* | ||
|
@@ -74,12 +92,21 @@ export interface MsalClient { | |
clientSecret: string, | ||
options?: GetTokenOptions, | ||
): Promise<AccessToken>; | ||
|
||
/** | ||
* Retrieves the last authenticated account. This method expects an authentication record to have been previously loaded. | ||
* | ||
* An authentication record could be loaded by calling the `getToken` method, or by providing an `authenticationRecord` when creating a credential. | ||
*/ | ||
getActiveAccount(): AuthenticationRecord | undefined; | ||
} | ||
|
||
/** | ||
* Options for creating an instance of the MsalClient. | ||
*/ | ||
export type MsalClientOptions = Partial<Omit<MsalNodeOptions, "clientId" | "tenantId">>; | ||
export type MsalClientOptions = Partial< | ||
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. Slowly working on defining this type. It's probably fine for now, but I'll clean this up soonish |
||
Omit<MsalNodeOptions, "clientId" | "tenantId" | "disableAutomaticAuthentication"> | ||
>; | ||
|
||
/** | ||
* Generates the configuration for MSAL (Microsoft Authentication Library). | ||
|
@@ -173,6 +200,40 @@ export function createMsalClient( | |
pluginConfiguration: msalPlugins.generatePluginConfiguration(createMsalClientOptions), | ||
}; | ||
|
||
const publicApps: Map<string, msal.PublicClientApplication> = new Map(); | ||
async function getPublicApp( | ||
options: GetTokenOptions = {}, | ||
): Promise<msal.PublicClientApplication> { | ||
const appKey = options.enableCae ? "CAE" : "default"; | ||
|
||
let publicClientApp = publicApps.get(appKey); | ||
if (publicClientApp) { | ||
msalLogger.getToken.info("Existing PublicClientApplication found in cache, returning it."); | ||
return publicClientApp; | ||
} | ||
|
||
// Initialize a new app and cache it | ||
msalLogger.getToken.info( | ||
`Creating new PublicClientApplication with CAE ${options.enableCae ? "enabled" : "disabled"}.`, | ||
); | ||
|
||
const cachePlugin = options.enableCae | ||
? state.pluginConfiguration.cache.cachePluginCae | ||
: state.pluginConfiguration.cache.cachePlugin; | ||
|
||
state.msalConfig.auth.clientCapabilities = options.enableCae ? ["cp1"] : undefined; | ||
|
||
publicClientApp = new msal.PublicClientApplication({ | ||
...state.msalConfig, | ||
broker: { nativeBrokerPlugin: state.pluginConfiguration.broker.nativeBrokerPlugin }, | ||
cache: { cachePlugin: await cachePlugin }, | ||
}); | ||
|
||
publicApps.set(appKey, publicClientApp); | ||
|
||
return publicClientApp; | ||
} | ||
|
||
const confidentialApps: Map<string, msal.ConfidentialClientApplication> = new Map(); | ||
async function getConfidentialApp( | ||
options: GetTokenOptions = {}, | ||
|
@@ -272,7 +333,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov | |
async function withSilentAuthentication( | ||
msalApp: msal.ConfidentialClientApplication | msal.PublicClientApplication, | ||
scopes: Array<string>, | ||
options: GetTokenOptions, | ||
options: GetTokenWithSilentAuthOptions, | ||
onAuthenticationRequired: () => Promise<msal.AuthenticationResult | null>, | ||
): Promise<AccessToken> { | ||
let response: msal.AuthenticationResult | null = null; | ||
|
@@ -282,7 +343,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov | |
if (e.name !== "AuthenticationRequiredError") { | ||
throw e; | ||
} | ||
if (createMsalClientOptions.disableAutomaticAuthentication) { | ||
if (options.disableAutomaticAuthentication) { | ||
throw new AuthenticationRequiredError({ | ||
scopes, | ||
getTokenOptions: options, | ||
|
@@ -324,14 +385,24 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov | |
|
||
const msalApp = await getConfidentialApp(options); | ||
|
||
return withSilentAuthentication(msalApp, scopes, options, () => | ||
msalApp.acquireTokenByClientCredential({ | ||
try { | ||
const response = await msalApp.acquireTokenByClientCredential({ | ||
scopes, | ||
authority: state.msalConfig.auth.authority, | ||
azureRegion: calculateRegionalAuthority(), | ||
claims: options?.claims, | ||
}), | ||
); | ||
}); | ||
ensureValidMsalToken(scopes, response, options); | ||
|
||
msalLogger.getToken.info(formatSuccess(scopes)); | ||
|
||
return { | ||
token: response.accessToken, | ||
expiresOnTimestamp: response.expiresOn.getTime(), | ||
}; | ||
} catch (err: any) { | ||
throw handleMsalError(scopes, err, options); | ||
} | ||
} | ||
|
||
async function getTokenByClientAssertion( | ||
|
@@ -345,15 +416,25 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov | |
|
||
const msalApp = await getConfidentialApp(options); | ||
|
||
return withSilentAuthentication(msalApp, scopes, options, () => | ||
msalApp.acquireTokenByClientCredential({ | ||
try { | ||
const response = await msalApp.acquireTokenByClientCredential({ | ||
scopes, | ||
authority: state.msalConfig.auth.authority, | ||
azureRegion: calculateRegionalAuthority(), | ||
claims: options?.claims, | ||
clientAssertion, | ||
}), | ||
); | ||
}); | ||
ensureValidMsalToken(scopes, response, options); | ||
|
||
msalLogger.getToken.info(formatSuccess(scopes)); | ||
|
||
return { | ||
token: response.accessToken, | ||
expiresOnTimestamp: response.expiresOn.getTime(), | ||
}; | ||
} catch (err: any) { | ||
throw handleMsalError(scopes, err, options); | ||
} | ||
} | ||
|
||
async function getTokenByClientCertificate( | ||
|
@@ -366,20 +447,66 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov | |
state.msalConfig.auth.clientCertificate = certificate; | ||
|
||
const msalApp = await getConfidentialApp(options); | ||
|
||
return withSilentAuthentication(msalApp, scopes, options, () => | ||
msalApp.acquireTokenByClientCredential({ | ||
try { | ||
const response = await msalApp.acquireTokenByClientCredential({ | ||
scopes, | ||
authority: state.msalConfig.auth.authority, | ||
azureRegion: calculateRegionalAuthority(), | ||
claims: options?.claims, | ||
}); | ||
ensureValidMsalToken(scopes, response, options); | ||
|
||
msalLogger.getToken.info(formatSuccess(scopes)); | ||
|
||
return { | ||
token: response.accessToken, | ||
expiresOnTimestamp: response.expiresOn.getTime(), | ||
}; | ||
} catch (err: any) { | ||
throw handleMsalError(scopes, err, options); | ||
} | ||
} | ||
|
||
async function getTokenByDeviceCode( | ||
scopes: string[], | ||
deviceCodeCallback: DeviceCodePromptCallback, | ||
options: GetTokenWithSilentAuthOptions = {}, | ||
): Promise<AccessToken> { | ||
msalLogger.getToken.info(`Attempting to acquire token using device code`); | ||
|
||
const msalApp = await getPublicApp(options); | ||
|
||
return withSilentAuthentication(msalApp, scopes, options, () => { | ||
const requestOptions: msal.DeviceCodeRequest = { | ||
scopes, | ||
cancel: false, | ||
deviceCodeCallback, | ||
authority: state.msalConfig.auth.authority, | ||
claims: options?.claims, | ||
}), | ||
); | ||
}; | ||
const deviceCodeRequest = msalApp.acquireTokenByDeviceCode(requestOptions); | ||
if (options.abortSignal) { | ||
options.abortSignal.addEventListener("abort", () => { | ||
maorleger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
requestOptions.cancel = true; | ||
}); | ||
} | ||
|
||
return deviceCodeRequest; | ||
}); | ||
} | ||
|
||
function getActiveAccount(): AuthenticationRecord | undefined { | ||
if (!state.cachedAccount) { | ||
return undefined; | ||
} | ||
return msalToPublic(clientId, state.cachedAccount); | ||
} | ||
|
||
return { | ||
getActiveAccount, | ||
getTokenByClientSecret, | ||
getTokenByClientAssertion, | ||
getTokenByClientCertificate, | ||
getTokenByDeviceCode, | ||
}; | ||
} |
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.
I understand that we want to not attempt silent authentication, but the naming of this parameter confuses me since you'd think you'd want to disable automatic authentication rather than setting this to false which would enable it?
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.
Yeah, I agree that the naming is confusing. The reason I stuck with it is because it matches the same behavior and meaning as the public API's version of
disableAutomaticAuthentication
which can help when searching the codebase for this.Personally I think a name like
disableInteractiveAuthentication
makes more sense, but I'm not sure if I want to stay consistent with the meaning of the public API field or do the name-mapping hereWhat this actually does, if I'm reading the source code correctly, is disable interactive auth - if an AuthenticationRequiredError is thrown from getTokenSilent and this option is true we will throw instead of continuing on to the interactive auth.
With that context in place - I don't have strong feelings and am happy to change it, but I have a slight preference to keeping the name and meaning consistent with the public API for better or worse. What do you think? @KarishmaGhiya would love your take 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.
@maorleger Is this param on L132 actually set by the public API param (L105) ? Or is this used for disabling interactive auth in another scenario (other than when passed in as a parameter by the user) ?
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 is used for enabling interactive auth fallback when the user passes is
disableAutomaticAuthentication: true
- so in thegetToken
call we will simply pass whatever the user passed in the public API param. Inauthenticate
we ignore what the user passed in becauseauthenticate
is meant to be called by the user whengetToken
fails to silent auth, and disableAutomaticAuthentication is true.Here's a few scenarios:
Happy to discuss more in our sync!