port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

sw-joelmut · 2021-09-07T20:22:11Z

Description

This PR ports the Managed Identity (MSI) and Single Tenant functionalities from DotNet.

Note: While testing the different combinations of the following table, we found that the case where a SingleTenant host connects a MultiTenant skill through expectReplies delivery mode works, the skill answered to the host properly, whereas using normal it didn't, due to not finding the resource in botframework.com (which is not supported, more information can be found in the related issue).

	MultiTenant Skill	SingleTenant Skill	MSI Skill
MultiTenant Host	Y	-	-
SingleTenant Host	normal (-), expectReplies (Y)	Y	Y
MSI Host	-	Y	Y

NOTE: When one of the bots is MultiTenant, the other cannot be SingleTenant/MSI since the token acquisition by the MultiTenant bot will fail due to not being able to find the SingleTenant/MSI bot resource registered in the botframework.com tenant. So, those scenarios are not supported.

Specific Changes

Updated ConfigurationServiceClientCredentialFactory to support the three MultiTenant, SingleTenant and MSI functionalities.
Added valid token issuers into the botbuilder-dialogs-adaptive-runtime and botframework-connector libraries.
Added new JwtTokenProviderFactory class that will be used to instantiate the DefaultAzureCredential class from @azure/identity.
Added ManagedIdentityAppCredentials class by extending from AppCredentials and be able to get the jwt token.
Added ManagedIdentityAuthenticator that will be in charge of creating the token provider and gather the token from it.
Added ManagedIdentityServiceClientCredentialsFactory class Factory in charge of creating the ManagedIdentityAppCredentials.
Added new unit tests for previous specified classes.
Added new/updated dependencies (package.json)
- botframework-connector
  - @azure/identity@2.0.0-beta.6
- root
  - @azure/logger@1.0.2
  - @azure/ms-rest-js@1.9.1

Testing

The following image shows some new added unit tests passing successfully.

Troubleshooting

@azure/identity
- When using the latest available version in NPM (1.5.2) we started getting Unexpected character... error on the keytar dependency when running yarn build.
- We found an issue related to keytar which will not be included in the base package in the 2.0.0 release.
  - Issue: https://github.com/Azure/azure-sdk-for-js/issues/14852#issuecomment-818915410
- Solution: Instead of installing the version 1.5.2 we use 2.0.0-beta.6 and no issues were shown, and the build was successful.
@azure/logger
- Package dependency of the @azure/identity library.
- Browserify fails to build the project due to not finding the @azure/logger library in the @azure/identity. This is caused by the browser configuration property set in the @azure/logger library.
- This issues was already solved in the newest versions, from which @azure/identity wasn't installing.
- Solution: By including the @azure/logger into the root package.json and setting the minimum version of ^1.0.2 solved the issue when building with browserify.

coveralls · 2021-09-07T20:36:51Z

Pull Request Test Coverage Report for Build 1279572164

79 of 85 (92.94%) changed or added relevant lines in 11 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 84.569%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
libraries/botframework-connector/src/auth/skillValidation.ts	1	2	50.0%
libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts	0	2	0.0%
libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts	21	24	87.5%

Files with Coverage Reduction	New Missed Lines	%
libraries/botframework-connector/src/auth/skillValidation.ts	1	87.61%
libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts	2	16.95%

Totals
Change from base Build 1258824352:	0.03%
Covered Lines:	19703
Relevant Lines:	22071

💛 - Coveralls

libraries/botframework-connector/src/auth/authenticationConfiguration.ts

libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts

libraries/botframework-connector/src/auth/managedIdentityAppCredentials.ts

libraries/botframework-connector/src/auth/managedIdentityServiceClientCredentialsFactory.ts

libraries/botframework-connector/src/auth/managedIdentityAuthenticator.ts

libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts

libraries/botframework-connector/src/auth/passwordServiceClientCredentialFactory.ts

package.json

joshgummersall · 2021-09-08T14:00:45Z

package.json

@@ -40,7 +40,8 @@
    "update-versions": "yarn workspace botbuilder-repo-utils update-versions"
  },
  "devDependencies": {
-    "@azure/ms-rest-js": "1.9.0",
+    "@azure/logger": "^1.0.2",
+    "@azure/ms-rest-js": "1.9.1",


This also should likely be reconciled with whatever package(s) it's used in. Why was this updated?

For consistency with other uses of the same dependency. We can revert it if you deem it so

…ation

…://github.com/southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

…b.com:southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

libraries/botframework-connector/src/auth/jwtTokenProviderFactory.ts

libraries/botframework-connector/src/auth/passwordServiceClientCredentialFactory.ts

libraries/botbuilder-core/package.json

…b.com:southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

…ailures

…it tests to validate

…-tenant-support' into southworks/add/port-msi-single-tenant-support

libraries/botbuilder-core/etc/botbuilder-core.api.md

libraries/botframework-connector/src/auth/managedIdentityAppCredentials.ts

joshgummersall

Little more feedback here, it's looking solid though. I pulled the branch and understand the root package.json changes better now, thanks for your patience while I convinced myself those changes were necessary.

libraries/botframework-connector/src/auth/skillValidation.ts

libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts

libraries/botframework-connector/src/auth/managedIdentityServiceClientCredentialsFactory.ts

libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts

libraries/botbuilder-core/package.json

libraries/botframework-connector/src/auth/managedIdentityAuthenticator.ts

libraries/botframework-connector/tests/auth/managedIdentityAuthenticator.test.js

libraries/botframework-connector/package.json

sw-joelmut · 2021-09-20T19:05:56Z

@joshgummersall Thank you for your feedback, we just pushed some changes, let us know if we can improve anything else 🤗(hug).

stevengum

The Identity team released @azure/identity@2.0.0-beta.6 which contains a few breaking changes that I don't believe affects the changes in this PR nor the rest of the botframework-connector package. That said, would you bump @azure/identity to this current next version and vet the contents of this PR again?

https://github.com/Azure/azure-sdk-for-js/blob/4868986e7f311d64478492c1571e59903ccbd3a1/sdk/identity/identity/CHANGELOG.md

libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts

sw-joelmut · 2021-09-21T20:01:29Z

The Identity team released @azure/identity@2.0.0-beta.6 which contains a few breaking changes that I don't believe affects the changes in this PR nor the rest of the botframework-connector package. That said, would you bump @azure/identity to this current next version and vet the contents of this PR again?

https://github.com/Azure/azure-sdk-for-js/blob/4868986e7f311d64478492c1571e59903ccbd3a1/sdk/identity/identity/CHANGELOG.md

Updated the @azure/identity to the latest 2.0.0-beta.6 and the functionality continues to work. Thanks, @stevengum.

joshgummersall

One final set of feedback

libraries/botframework-connector/src/auth/managedIdentityAppCredentials.ts

libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts

libraries/botframework-connector/src/auth/parameterizedBotFrameworkAuthentication.ts

libraries/botframework-connector/src/auth/skillValidation.ts

stevengum · 2021-09-23T22:12:55Z

libraries/botframework-connector/src/auth/authenticationConstants.ts

+    /**
+     * The Government V1 Azure AD token issuer URL template that will contain the tenant id where the token was issued from.
+     */
+    export const ValidGovernmentTokenIssuerUrlTemplateV1 = 'https://login.microsoftonline.us/';
+
+    /**
+     * The Government V2 Azure AD token issuer URL template that will contain the tenant id where the token was issued from.
+     */
+    export const ValidGovernmentTokenIssuerUrlTemplateV2 = 'https://login.microsoftonline.us/';


Shouldn't this be in the GovernmentConstants instead? @msomanathan

It's not a big deal, but historically all of the US Gov Cloud related constants are stored in a separate file. My main concern is the v4-to-v5 work and making sure our ducks are in a row.

I did note that this is a verbatim port from .NET.

Filed microsoft/botframework-sdk#6384 for tracking.

libraries/botframework-connector/src/auth/jwtTokenProviderFactoryInterface.ts

libraries/botframework-connector/src/auth/managedIdentityAppCredentials.ts

libraries/botframework-connector/src/auth/managedIdentityAuthenticator.ts

libraries/botframework-connector/tests/auth/managedIdentityAuthenticator.test.js

...ies/botframework-connector/tests/auth/managedIdentityServiceClientCredentialsFactory.test.js

sw-joelmut · 2021-09-24T20:42:47Z

@stevengum Thank you for the feedback, we just pushed the changes.
There are some additional changes applied based on the provided feedback, let us know if there is anything else we could improve on the implementation. Thanks.

joshgummersall

Approved, pending test pass!

libraries/botframework-connector/src/auth/jwtTokenProviderFactory.ts

ceciliaavila and others added 8 commits September 2, 2021 11:43

Port MSI and SingleTenant support

2253145

Add unit tests for botframework-connector

189f9d2

Restore yarn.lock

d88c616

Test nohoist for azure/identity

7c45a2b

Fix dependencies issues

542a7ab

Connect ManagedIdentityAuthenticator.getToken

f3dd2c3

Add remaining tests and fix the already added

86f0f2d

Update yarn.lock

e0a233a

sw-joelmut requested a review from mrivera-ms September 7, 2021 20:22

mrivera-ms requested a review from joshgummersall September 7, 2021 21:10

joshgummersall reviewed Sep 8, 2021

View reviewed changes

MartinLuccanera and others added 7 commits September 9, 2021 09:29

Fixing depcheck errors in PR

171733b

Fixing depcheck, adjusting versions and ignores

875b0d3

Fix parity port with DotNet causing SingleTenant to fail at authentic…

288765f

…ation

Merge branch 'southworks/add/port-msi-single-tenant-support' of https…

0c4a9a6

…://github.com/southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

Apppliying feedback

135522d

Merge branch 'southworks/add/port-msi-single-tenant-support' of githu…

14386eb

…b.com:southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

Applying fixes for tests, lint and zod related feedback

afebb4d

joshgummersall reviewed Sep 13, 2021

View reviewed changes

libraries/botframework-connector/src/auth/jwtTokenProviderFactory.ts Outdated Show resolved Hide resolved

libraries/botframework-connector/src/auth/passwordServiceClientCredentialFactory.ts Show resolved Hide resolved

libraries/botbuilder-core/package.json Outdated Show resolved Hide resolved

MartinLuccanera added 4 commits September 14, 2021 12:14

Fixing asserts import and extra parameter

bc25476

Merge branch 'southworks/add/port-msi-single-tenant-support' of githu…

799842f

…b.com:southworks/botbuilder-js into southworks/add/port-msi-single-tenant-support

Fix tests failing due to wrong link and fixing case-wise comparison f…

a6132b6

…ailures

Fixing lint

5c95709

ceciliaavila changed the title ~~[DRAFT][#3906] Port Managed Identity (MSI) + Single Tenant from DotNet~~ port: [DRAFT][#3906] Port Managed Identity (MSI) + Single Tenant from DotNet Sep 15, 2021

ceciliaavila and others added 4 commits September 15, 2021 11:00

Merge branch 'main' into southworks/add/port-msi-single-tenant-support

4d54c25

Add ignore-casing to MicrosoftAppType based on DotNet code and add un…

12fe13f

…it tests to validate

Improve issuer array assignation

c78713a

Merge remote-tracking branch 'upstream/southworks/add/port-msi-single…

7bbf539

…-tenant-support' into southworks/add/port-msi-single-tenant-support

sw-joelmut marked this pull request as ready for review September 16, 2021 20:05

MartinLuccanera changed the title ~~port: [DRAFT][#3906] Port Managed Identity (MSI) + Single Tenant from DotNet~~ port: [#3906] Port Managed Identity (MSI) + Single Tenant from DotNet Sep 16, 2021

mrivera-ms changed the title ~~port: [#3906] Port Managed Identity (MSI) + Single Tenant from DotNet~~ port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet Sep 16, 2021

joshgummersall reviewed Sep 17, 2021

View reviewed changes

libraries/botbuilder-core/etc/botbuilder-core.api.md Outdated Show resolved Hide resolved

joshgummersall reviewed Sep 17, 2021

View reviewed changes

libraries/botframework-connector/src/auth/managedIdentityAppCredentials.ts Outdated Show resolved Hide resolved

fix: export ms-rest-js type and remove added dep

0e207e5

joshgummersall reviewed Sep 17, 2021

View reviewed changes

libraries/botframework-connector/package.json Outdated Show resolved Hide resolved

sw-joelmut added 2 commits September 20, 2021 13:31

Improve issuers, assertions, unit tests and many other small changes

9f75226

fix: skillValidation validTokenIssuers

27fbdf0

Merge branch 'main' into southworks/add/port-msi-single-tenant-support

1fa5179

stevengum reviewed Sep 21, 2021

View reviewed changes

libraries/botbuilder-core/src/configurationServiceClientCredentialFactory.ts Show resolved Hide resolved

Update @azure/identity to 2.0.0-beta.6

a8a8277

joshgummersall reviewed Sep 21, 2021

View reviewed changes

Change logical operator for nullable operator

9c9f8d9

stevengum requested changes Sep 23, 2021

View reviewed changes

sw-joelmut added 2 commits September 24, 2021 16:03

Merge branch 'main' into southworks/add/port-msi-single-tenant-support

2f88d99

Improve SingleTenant and MSI implementation from feedback

3245f18

Josh Gummersall added 2 commits September 27, 2021 11:44

fix: IJwtTokenProviderFactory

47d891f

fix: reintroduced typo

5f66d89

joshgummersall approved these changes Sep 27, 2021

View reviewed changes

joshgummersall requested a review from stevengum September 27, 2021 18:48

joshgummersall mentioned this pull request Sep 27, 2021

Reconcile auth constants organization microsoft/botframework-sdk#6384

Closed

stevengum reviewed Sep 27, 2021

View reviewed changes

libraries/botframework-connector/src/auth/jwtTokenProviderFactory.ts Show resolved Hide resolved

stevengum approved these changes Sep 27, 2021

View reviewed changes

stevengum merged commit edb0f90 into main Sep 27, 2021

stevengum deleted the southworks/add/port-msi-single-tenant-support branch September 27, 2021 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

sw-joelmut commented Sep 7, 2021 •

edited

Loading

coveralls commented Sep 7, 2021 •

edited

Loading

joshgummersall Sep 8, 2021

MartinLuccanera Sep 10, 2021

joshgummersall left a comment

sw-joelmut commented Sep 20, 2021

stevengum left a comment

sw-joelmut commented Sep 21, 2021 •

edited

Loading

joshgummersall left a comment

stevengum Sep 23, 2021

joshgummersall Sep 27, 2021

sw-joelmut commented Sep 24, 2021

joshgummersall left a comment

port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

port:[#3906] Port Managed Identity (MSI) + Single Tenant from DotNet #3923

Conversation

sw-joelmut commented Sep 7, 2021 • edited Loading

Description

Specific Changes

Testing

Troubleshooting

coveralls commented Sep 7, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1279572164

💛 - Coveralls

joshgummersall Sep 8, 2021

Choose a reason for hiding this comment

MartinLuccanera Sep 10, 2021

Choose a reason for hiding this comment

joshgummersall left a comment

Choose a reason for hiding this comment

sw-joelmut commented Sep 20, 2021

stevengum left a comment

Choose a reason for hiding this comment

sw-joelmut commented Sep 21, 2021 • edited Loading

joshgummersall left a comment

Choose a reason for hiding this comment

stevengum Sep 23, 2021

Choose a reason for hiding this comment

joshgummersall Sep 27, 2021

Choose a reason for hiding this comment

sw-joelmut commented Sep 24, 2021

joshgummersall left a comment

Choose a reason for hiding this comment

sw-joelmut commented Sep 7, 2021 •

edited

Loading

coveralls commented Sep 7, 2021 •

edited

Loading

sw-joelmut commented Sep 21, 2021 •

edited

Loading