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: [#4449] CloudAdapter always builds Connector with MicrosoftAppCredentials (never CertificateAppCredentials) -- certificate auth flow broken #4457

Merged
merged 2 commits into from
May 5, 2023

Conversation

sw-joelmut
Copy link
Collaborator

Fixes #4449

Description

This PR adds support for the CloudAdapter's configuration process to authenticate using certificates is MultiTenant and SingleTenant apps.

Specific Changes

  • Added CertificateServiceClientCredentialsFactory class, to instantiate and configure the CertificateAppCredentials class, so the process knows how to authenticate with certificates.
  • Added ConfigurationServiceClientCredentialFactory unit tests, validating the different code paths when using certificates.
  • Added CertificateThumbprint and CertificatePrivateKey constants to the AuthenticationConstants, ConfigurationBotFrameworkAuthentication and ConfigurationServiceClientCredentialFactory classes.
  • Updated ConfigurationServiceClientCredentialFactory to detect when a certificate is provided, and execute the proper functionality to authenticate with certificates (instead of a password) in MultiTenant and SingleTenant apps.

Testing

The following images show the bot and unit tests working as expected.
image
image

@sw-joelmut sw-joelmut added the Automation: Parity with dotnet The PR needs to be ported to dotnet. label Apr 21, 2023
@sw-joelmut sw-joelmut requested a review from a team as a code owner April 21, 2023 17:55
@coveralls
Copy link

coveralls commented Apr 21, 2023

Pull Request Test Coverage Report for Build 4767518035

  • 30 of 31 (96.77%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 84.672%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts 15 16 93.75%
Totals Coverage Status
Change from base Build 4198805038: 0.02%
Covered Lines: 20041
Relevant Lines: 22432

💛 - Coveralls

@jineshjin
Copy link

@sw-joelmut when can we expect this to be reviewed, merged and released?

@sw-joelmut
Copy link
Collaborator Author

Hi @jineshjin, I don't know when it will be released, maybe @tracyboehrer can help us with that.

@jineshjin
Copy link

Hi @tracyboehrer, just wanted to know, when are we planning to merge and release this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation: Parity with dotnet The PR needs to be ported to dotnet.
Projects
None yet
4 participants