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

feat: cloud adapter #3790

Merged
merged 6 commits into from
Jun 23, 2021
Merged

feat: cloud adapter #3790

merged 6 commits into from
Jun 23, 2021

Conversation

joshgummersall
Copy link
Contributor

@joshgummersall joshgummersall commented Jun 22, 2021

All code is reviewed save for #3789, leave any feedback on that PR and I will address retroactively here.

Fixes several issues, all linked inline to the commits.

#minor

stevengum and others added 4 commits June 22, 2021 14:54
…ter (#3698)

* add additional BotFrameworkAuthentication classes for CloudAdapter

* Add Alexa to Channels enum for #3603

* fix export, USER_AGENT and style issue

* make path in Configuration.get optional, apply other PR feedback

* address additional PR feedback, minor cleanup

* update adaptive-runtime.Configuration to reflect contract

* cleanup typing, add unit tests, refactor existing code

* fix linting error in types.ts
* remove Cortana from Channels enum

* clean up linted choices_* tests
* port: cloud adapter base

- remove unnecessary emitter type
- streaming syntax issues
- introduce runtypes helpers
- bot framework adapter implements http adapter interface

* add created status code, remove dupe status codes

Not sure how the duplicate status codes piece was introduced, seems unnecessary though.

* add continue conv w/ claims to bot adapter

This enables us to properly pass claims to cloud adapter without plumbing them
all the way through turn context. This is handled in .NET via overloads, however
we can't achieve the same thing due to backwards-compat requirements in BotAdapter.

See .NET overloads:
https://github.com/microsoft/botbuilder-dotnet/blob/3c335046f95deeac50fbb0b48c7c8c42051d4f6d/libraries/Microsoft.Bot.Builder/CloudAdapterBase.cs#L147-L163

* connectory factory create -> audience?

* cloud adapter + associated runtime changes

- properly handle streaming requests
- include retries for named pipes

* cloud skill handler changes

- better http errors for channel service routes
- clean up channel service handler tests

This is essentially a port of microsoft/botbuilder-dotnet#5304

* better docs/comments all around

* plumb errors through to http responses

- req + res in turn context, use in adapter error handler
- fix several errors that should have yielded a 401

* port builtin auth handler logic

This accounts for the no-auth (Emulator) skills scenario. The logic exists in .NET here:
https://github.com/microsoft/botbuilder-dotnet/blob/3c335046f95deeac50fbb0b48c7c8c42051d4f6d/libraries/Microsoft.Bot.Connector/Authentication/BuiltinBotFrameworkAuthentication.cs#L72-L87

* fix anonymous auth handling

The default scenario after creating a bot using Composer is a settings file where `MicrosoftAppId` is set to `""`, aka
the empty string. When Emulator sends requests without an app ID/password, the auth header is missing and thus produces
a null app ID (identity.getClaims returns null). This commit properly accounts for this anonymous auth scenario.

* fix runtime constructor exception

The default runtime config does not set any of the configuration values related to auth. As a result, runtypes checking
fails. This change coerces the runtype into a partial (to match the contructor signature) and assigns the checked value.

Also,

- fix boolean coercian to stringify value
- use import type

* small runtypes cleanup

Also remove unnecessary "as" casting

* remove extra call to isValidAppId

This call does not exist in the .NET counterpart:
https://github.com/microsoft/botbuilder-dotnet/blob/main/libraries/Microsoft.Bot.Connector/Authentication/PasswordServiceClientCredentialFactory.cs#L76

* fix dialog context error message issue

* cloud adapter base -> core

Also extract response error handling to integrations as base no longer knows about HTTP specifics

* user token access helper

Also, refactor adaptive oauth input using oauth prompt

* reconcile core bot adapter changes

* handle streaming version request

Note that token handling also does not exist in .NET:
https://github.com/microsoft/botbuilder-dotnet/blob/main/libraries/Microsoft.Bot.Builder/Streaming/StreamingRequestHandler.cs#L517

* support service client credentials factory in runtime

* restore appId validation to pwServiceCredentialFactory.createCredentials()
- remove "only-warn" plugin
- add tests

* quick PR feedback fixes

* remove duplicate call to create connector client

* continueConversationAsync with overloads

* finishing touches

- remaining PR items
- remove BotLogic type
- test:compat fixes

Co-authored-by: stevengum <14935595+stevengum@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Jun 23, 2021

Pull Request Test Coverage Report for Build 962613302

  • 398 of 779 (51.09%) changed or added relevant lines in 45 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.6%) to 83.79%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botbuilder-azure-blobs/src/blobsTranscriptStore.ts 0 1 0.0%
libraries/botbuilder-dialogs-adaptive/src/input/oauthInput.ts 2 3 66.67%
libraries/botbuilder-dialogs/src/dialogContextError.ts 3 4 75.0%
libraries/botbuilder/src/interfaces/response.ts 2 3 66.67%
libraries/botbuilder/src/skills/skillHandler.ts 9 10 90.0%
libraries/botbuilder/src/zod.ts 4 5 80.0%
libraries/botframework-connector/src/auth/userTokenClientImpl.ts 2 3 66.67%
libraries/botframework-schema/src/activityEx.ts 2 3 66.67%
libraries/botbuilder/src/channelServiceRoutes.ts 1 3 33.33%
libraries/botframework-connector/src/auth/passwordServiceClientCredentialFactory.ts 28 30 93.33%
Files with Coverage Reduction New Missed Lines %
libraries/botbuilder/src/channelServiceRoutes.ts 1 82.43%
libraries/botbuilder-dialogs/src/prompts/oauthPrompt.ts 3 78.57%
libraries/botbuilder-dialogs-adaptive/src/input/oauthInput.ts 8 63.68%
Totals Coverage Status
Change from base Build 961387807: -1.6%
Covered Lines: 19494
Relevant Lines: 22039

💛 - Coveralls

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@stevengum stevengum merged commit 636daf8 into main Jun 23, 2021
@stevengum stevengum deleted the cloud-adapter branch June 23, 2021 04:01
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.

3 participants