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

Conversation

sw-joelmut
Copy link
Collaborator

@sw-joelmut sw-joelmut commented Oct 20, 2023

Fixes #2782

Description

This PR removes the deprecated adal-node dependency and all related code, in favor of using @azure/msal-node.
We have updated all internal code structure related to bot authentication when using ADAL to start using MSAL instead. These changes include a refactor to be more closely similar to DotNet's implementation.
Additionally, we took the opportunity to add the missing x5c (SNI) parameter value for Certificate authentication.

Specific Changes

  • Removed adal-node references and dependencies.
  • AppCredentials
    • New AuthenticatorResult interface in favor of adal.TokenResponse to avoid future type breaking changes.
    • Improved way to cache the acquired token.
  • CertificateAppCredentials
    • Added x5c (SNI) parameter to be passed in to MSAL.
    • Delegate getting the token functionality to the new MSAL authentication class.
  • ManagedIdentityAppCredentials
    • Cleaned up unnecessary properties from the refreshToken result.
  • MicrosoftAppCredentials
    • Delegate getting the token functionality to the new MSAL authentication class.
  • MsalAppCredentials
    • Cleaned up unnecessary properties from the refreshToken result.
  • Updated multiple unit tests around nock mocks because of the new MSAL implementation behaves differently than ADAL.

Testing

The following image shows the Skills' bots working after the changes, and how we tested using certificates with the new x5c parameter.
image
image

@sw-joelmut sw-joelmut requested a review from a team as a code owner October 20, 2023 19:58
@coveralls
Copy link

coveralls commented Oct 20, 2023

Pull Request Test Coverage Report for Build 6592142434

  • 17 of 24 (70.83%) changed or added relevant lines in 7 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 84.802%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botframework-connector/src/auth/appCredentials.ts 6 7 85.71%
libraries/botframework-connector/src/auth/microsoftAppCredentials.ts 2 4 50.0%
libraries/botframework-connector/src/auth/certificateAppCredentials.ts 3 7 42.86%
Files with Coverage Reduction New Missed Lines %
libraries/botframework-connector/src/auth/passwordServiceClientCredentialFactory.ts 1 90.0%
testing/botbuilder-test-utils/src/oauth.ts 7 32.0%
Totals Coverage Status
Change from base Build 6580388819: 0.03%
Covered Lines: 20269
Relevant Lines: 22645

💛 - Coveralls

@sw-joelmut
Copy link
Collaborator Author

Hi @tracyboehrer,

This is the PR containing all the changes that remove ADAL in favor of MSAL.

@XiaofuHuang
Copy link

Is it feasible to employ an upgraded version of @azure/msal-node to accommodate Node.js 20?

@tracyboehrer tracyboehrer merged commit 593cd3b into main Oct 23, 2023
14 checks passed
@tracyboehrer tracyboehrer deleted the southworks/update/remove-adal-refs branch October 23, 2023 13:48
@ceciliaavila
Copy link
Collaborator

Is it feasible to employ an upgraded version of @azure/msal-node to accommodate Node.js 20?

Hi @XiaofuHuang, @azure/msal-node version 2.x is not compatible with node versions 14 and 16 which the SDK still supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to MSAL from adal-node
5 participants