-
Notifications
You must be signed in to change notification settings - Fork 6
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
[JS] Add details to README in certificate samples #427
Conversation
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.
Similar changes made for .NET could be made here #429.
Also, the linked images are not showing up in the readme.
samples/javascript_nodejs/84.bot-authentication-certificate/README.md
Outdated
Show resolved
Hide resolved
samples/javascript_nodejs/84.bot-authentication-certificate/README.md
Outdated
Show resolved
Hide resolved
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.
Check casing of links.
If repo is cloned in linux/unix system, then Images
is not the same as images
.
samples/javascript_nodejs/84.bot-authentication-certificate/README.md
Outdated
Show resolved
Hide resolved
Changes applied |
@@ -1,4 +1,3 @@ | |||
MicrosoftAppTenantId= |
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 noticed we dont have the MicrosoftAppTenantId and MicrosoftAppType, whe should include them here in the .env and the index.js, because they could be used with this flow path.
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.
MicrosoftAppType doesn't exist in ConfigurationBotFrameworkAuthenticationOptions
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.
Also does not exist in CertificateServiceClientCredentialsFactory(SN+I)
https://github.com/microsoft/botbuilder-js/blob/066b405a8630e4f6c7d165ef939982181f6bbc6a/libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts#L41
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.
added Tenant Id
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.
The app type should be added as well, i know its not in the ConfigurationBotFrameworkAuthenticationOptions, but it is in the https://github.com/microsoft/botbuilder-js/blob/066b405a8630e4f6c7d165ef939982181f6bbc6a/libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts which is the one that instantiates the CertificateServiceClientCredentialsFactory
Promoted /3994 |
#minor
Description
This PR adds more information to the README certificate samples to have an easier complete guide for SSL/TLS certificate configuration.
Proposed Changes