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

fix: [#2782] Migrate to MSAL from adal-node #4548

Merged
merged 1 commit into from
Oct 23, 2023
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
1 change: 1 addition & 0 deletions libraries/botbuilder/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
},
"dependencies": {
"@azure/ms-rest-js": "^2.7.0",
"@azure/msal-node": "^1.2.0",
"axios": "^0.25.0",
"botbuilder-core": "4.1.6",
"botbuilder-stdlib": "4.1.6",
Expand Down
158 changes: 64 additions & 94 deletions libraries/botbuilder/tests/teamsInfo.test.js

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion libraries/botframework-connector/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
"@azure/identity": "^2.0.4",
"@azure/ms-rest-js": "^2.7.0",
"@azure/msal-node": "^1.2.0",
"adal-node": "0.2.3",
"axios": "^0.25.0",
"base64url": "^3.0.0",
"botbuilder-stdlib": "4.1.6",
Expand Down
40 changes: 14 additions & 26 deletions libraries/botframework-connector/src/auth/appCredentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,24 @@
*/

import * as msrest from '@azure/ms-rest-js';
import * as adal from 'adal-node';
import { ConfidentialClientApplication } from '@azure/msal-node';
import { AuthenticationConstants } from './authenticationConstants';
import { AuthenticatorResult } from './authenticatorResult';

/**
* General AppCredentials auth implementation and cache. Supports any ADAL client credential flow.
* General AppCredentials auth implementation and cache.
* Subclasses can implement refreshToken to acquire the token.
*/
export abstract class AppCredentials implements msrest.ServiceClientCredentials {
private static readonly cache: Map<string, adal.TokenResponse> = new Map<string, adal.TokenResponse>();
private static readonly cache: Map<string, AuthenticatorResult> = new Map<string, AuthenticatorResult>();

appId: string;

private _oAuthEndpoint: string;
private _oAuthScope: string;
private _tenant: string;
tokenCacheKey: string;
protected refreshingToken: Promise<adal.TokenResponse> | null = null;
protected authenticationContext: adal.AuthenticationContext;
protected clientApplication: ConfidentialClientApplication;

// Protects against JSON.stringify leaking secrets
private toJSON(): unknown {
Expand Down Expand Up @@ -104,7 +104,6 @@ export abstract class AppCredentials implements msrest.ServiceClientCredentials
// aadApiVersion is set to '1.5' to avoid the "spn:" concatenation on the audience claim
// For more info, see https://github.com/AzureAD/azure-activedirectory-library-for-nodejs/issues/128
this._oAuthEndpoint = value;
this.authenticationContext = new adal.AuthenticationContext(value, true, undefined, '1.5');
}

/**
Expand Down Expand Up @@ -168,41 +167,30 @@ export abstract class AppCredentials implements msrest.ServiceClientCredentials
async getToken(forceRefresh = false): Promise<string> {
if (!forceRefresh) {
// check the global cache for the token. If we have it, and it's valid, we're done.
const oAuthToken: adal.TokenResponse = AppCredentials.cache.get(this.tokenCacheKey);
if (oAuthToken) {
// we have the token. Is it valid?
if (oAuthToken.expirationTime > Date.now()) {
return oAuthToken.accessToken;
}
const oAuthToken = AppCredentials.cache.get(this.tokenCacheKey);
// Check if the token is not expired.
if (oAuthToken && oAuthToken.expiresOn > new Date()) {
return oAuthToken.accessToken;
}
}

// We need to refresh the token, because:
// 1. The user requested it via the forceRefresh parameter
// 2. We have it, but it's expired
// 3. We don't have it in the cache.
const res: adal.TokenResponse = await this.refreshToken();
this.refreshingToken = null;
const res = await this.refreshToken();

if (res && res.accessToken) {
// `res` is equalivent to the results from the cached promise `this.refreshingToken`.
// Because the promise has been cached, we need to see if the body has been read.
// If the body has not been read yet, we can call res.json() to get the access_token.
// If the body has been read, the OAuthResponse for that call should have been cached already,
// in which case we can return the cache from there. If a cached OAuthResponse does not exist,
// call getToken() again to retry the authentication process.

// Subtract 5 minutes from expires_in so they'll we'll get a
// new token before it expires.
res.expirationTime = Date.now() + res.expiresIn * 1000 - 300000;
// Subtract 5 minutes from expiresOn so they'll we'll get a new token before it expires.
res.expiresOn.setMinutes(res.expiresOn.getMinutes() - 5);
AppCredentials.cache.set(this.tokenCacheKey, res);
return res.accessToken;
} else {
throw new Error('Authentication: No response or error received from ADAL.');
throw new Error('Authentication: No response or error received from MSAL.');
}
}

protected abstract refreshToken(): Promise<adal.TokenResponse>;
protected abstract refreshToken(): Promise<AuthenticatorResult>;

/**
* @private
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/

/**
* Contains tokens and metadata upon successful completion of an acquireToken call.
*/
export interface AuthenticatorResult {
/**
* The value of the access token resulting from an authentication process.
*/
accessToken: string;
/**
* The date and time of expiration relative to Coordinated Universal Time (UTC).
*/
expiresOn: Date;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,20 @@
* Licensed under the MIT License.
*/

import * as adal from 'adal-node';
import { ConfidentialClientApplication } from '@azure/msal-node';
import { AppCredentials } from './appCredentials';
import { AuthenticatorResult } from './authenticatorResult';
import { MsalAppCredentials } from './msalAppCredentials';

/**
* CertificateAppCredentials auth implementation
*/
export class CertificateAppCredentials extends AppCredentials {
certificateThumbprint: string;
certificatePrivateKey: string;
x5c: string;

private credentials: MsalAppCredentials;

/**
* Initializes a new instance of the [CertificateAppCredentials](xref:botframework-connector.CertificateAppCredentials) class.
Expand All @@ -24,37 +29,55 @@ export class CertificateAppCredentials extends AppCredentials {
* @param certificatePrivateKey A PEM encoded certificate private key.
* @param channelAuthTenant Optional. The oauth token tenant.
* @param oAuthScope Optional. The scope for the token.
* @param x5c Optional. Enables application developers to achieve easy certificates roll-over in Azure AD:
* set this parameter to send the public certificate (BEGIN CERTIFICATE) to Azure AD, so that Azure AD can use it to validate the subject name based on a trusted issuer policy.
*/
constructor(
appId: string,
certificateThumbprint: string,
certificatePrivateKey: string,
channelAuthTenant?: string,
oAuthScope?: string
oAuthScope?: string,
x5c?: string
) {
super(appId, channelAuthTenant, oAuthScope);
this.certificateThumbprint = certificateThumbprint;
this.certificatePrivateKey = certificatePrivateKey;
this.x5c = x5c;
}

/**
* @inheritdoc
*/
async getToken(forceRefresh = false): Promise<string> {
this.credentials ??= new MsalAppCredentials(
this.createClientApplication(),
this.appId,
this.oAuthEndpoint,
this.oAuthScope
);
return this.credentials.getToken(forceRefresh);
}

/**
* @inheritdoc
*/
protected refreshToken(): Promise<AuthenticatorResult> {
// This will never be executed because we are using MsalAppCredentials.getToken underneath.
throw new Error('Method not implemented.');
}

protected async refreshToken(): Promise<adal.TokenResponse> {
if (!this.refreshingToken) {
this.refreshingToken = new Promise<adal.TokenResponse>((resolve, reject) => {
this.authenticationContext.acquireTokenWithClientCertificate(
this.oAuthScope,
this.appId,
this.certificatePrivateKey,
this.certificateThumbprint,
function (err, tokenResponse) {
if (err) {
reject(err);
} else {
resolve(tokenResponse as adal.TokenResponse);
}
}
);
});
}
return this.refreshingToken;
private createClientApplication() {
return new ConfidentialClientApplication({
auth: {
clientId: this.appId,
authority: this.oAuthEndpoint,
clientCertificate: {
thumbprint: this.certificateThumbprint,
privateKey: this.certificatePrivateKey,
x5c: this.x5c,
},
},
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
private readonly certificateThumbprint: string;
private readonly certificatePrivateKey: string;
private readonly tenantId: string | null;
private readonly x5c: string | null;

/**
* Initializes a new instance of the CertificateServiceClientCredentialsFactory class.
Expand All @@ -27,8 +28,16 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
* @param certificateThumbprint A hex encoded thumbprint of the certificate.
* @param certificatePrivateKey A PEM encoded certificate private key.
* @param tenantId Optional. The oauth token tenant.
* @param x5c Optional. Enables application developers to achieve easy certificates roll-over in Azure AD:
* set this parameter to send the public certificate (BEGIN CERTIFICATE) to Azure AD, so that Azure AD can use it to validate the subject name based on a trusted issuer policy.
*/
constructor(appId: string, certificateThumbprint: string, certificatePrivateKey: string, tenantId?: string) {
constructor(
appId: string,
certificateThumbprint: string,
certificatePrivateKey: string,
tenantId?: string,
x5c?: string
) {
super();
ok(appId?.trim(), 'CertificateServiceClientCredentialsFactory.constructor(): missing appId.');
ok(
Expand All @@ -44,6 +53,7 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
this.certificateThumbprint = certificateThumbprint;
this.certificatePrivateKey = certificatePrivateKey;
this.tenantId = tenantId;
this.x5c = x5c;
}

/**
Expand Down Expand Up @@ -75,7 +85,8 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
this.certificateThumbprint,
this.certificatePrivateKey,
this.tenantId,
audience
audience,
this.x5c
);
}
}
5 changes: 3 additions & 2 deletions libraries/botframework-connector/src/auth/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@

export * from './allowedCallersClaimsValidator';
export * from './appCredentials';
export * from './authenticateRequestResult';
export * from './authenticationConfiguration';
export * from './authenticationConstants';
export * from './authenticationError';
export * from './authenticateRequestResult';
export * from './authenticatorResult';
export * from './botFrameworkAuthentication';
export * from './botFrameworkAuthenticationFactory';
export * from './certificateAppCredentials';
Expand All @@ -32,8 +33,8 @@ export * from './managedIdentityAuthenticator';
export * from './managedIdentityServiceClientCredentialsFactory';
export * from './microsoftAppCredentials';
export * from './passwordServiceClientCredentialFactory';
export * from './skillValidation';
export * from './serviceClientCredentialsFactory';
export * from './skillValidation';
export * from './userTokenClient';

export { MsalAppCredentials } from './msalAppCredentials';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
* Licensed under the MIT License.
*/

import { ok } from 'assert';
import { AppCredentials } from './appCredentials';
import type { IJwtTokenProviderFactory } from './jwtTokenProviderFactory';
import { ManagedIdentityAuthenticator } from './managedIdentityAuthenticator';
import { TokenResponse } from 'adal-node';
import { ok } from 'assert';
import { AuthenticatorResult } from './authenticatorResult';

/**
* Managed Service Identity auth implementation.
Expand Down Expand Up @@ -40,14 +40,11 @@ export class ManagedIdentityAppCredentials extends AppCredentials {
/**
* @inheritdoc
*/
protected async refreshToken(): Promise<TokenResponse> {
protected async refreshToken(): Promise<AuthenticatorResult> {
const token = await this.authenticator.getToken();
return {
accessToken: token.token,
expiresOn: new Date(token.expiresOnTimestamp),
tokenType: 'Bearer',
expiresIn: (token.expiresOnTimestamp - Date.now()) / 1000,
resource: this.oAuthScope,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,9 @@
* Licensed under the MIT License.
*/

import * as adal from 'adal-node';
import { AppCredentials } from './appCredentials';

// Determines if an unknown value is of adal.ErrorResponse type
function isErrorResponse(value: unknown): value is adal.ErrorResponse {
if (value) {
const { error, errorDescription } = value as adal.ErrorResponse;
return error != null && errorDescription != null;
}

return false;
}
import { AuthenticatorResult } from './authenticatorResult';
import { MsalAppCredentials } from './msalAppCredentials';

/**
* MicrosoftAppCredentials auth implementation
Expand All @@ -28,6 +19,8 @@ export class MicrosoftAppCredentials extends AppCredentials {
*/
static readonly Empty = new MicrosoftAppCredentials(null, null);

private credentials: MsalAppCredentials;

/**
* Initializes a new instance of the [MicrosoftAppCredentials](xref:botframework-connector.MicrosoftAppCredentials) class.
*
Expand All @@ -40,26 +33,19 @@ export class MicrosoftAppCredentials extends AppCredentials {
super(appId, channelAuthTenant, oAuthScope);
}

protected async refreshToken(): Promise<adal.TokenResponse> {
if (!this.refreshingToken) {
this.refreshingToken = new Promise<adal.TokenResponse>((resolve, reject): void => {
this.authenticationContext.acquireTokenWithClientCredentials(
this.oAuthScope,
this.appId,
this.appPassword,
(err, tokenResponse) => {
if (err) {
reject(err);
} else if (isErrorResponse(tokenResponse)) {
reject(new Error(tokenResponse.error));
} else {
resolve(tokenResponse);
}
}
);
});
}
/**
* @inheritdoc
*/
async getToken(forceRefresh = false): Promise<string> {
this.credentials ??= new MsalAppCredentials(this.appId, this.appPassword, this.oAuthEndpoint, this.oAuthScope);
return this.credentials.getToken(forceRefresh);
}

return this.refreshingToken;
/**
* @inheritdoc
*/
protected refreshToken(): Promise<AuthenticatorResult> {
// This will never be executed because we are using MsalAppCredentials.getToken underneath.
throw new Error('Method not implemented.');
}
}
Loading