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

Move Error logic out of botbuilder into authentication layer #1985

Merged
merged 34 commits into from
Apr 22, 2020

Conversation

Zerryth
Copy link
Contributor

@Zerryth Zerryth commented Apr 2, 2020

Fixes #1445

Description

See discussion in comment.

  • Create custom error class that throws richer errors, so we do not have to rely on parsing the error message to determine the correct status code.
  • Move the logic out of botbuilder into the authentication layer

Specific Changes

  • Create IStatusCodeError in botbuilder-schema.
    • Currently there's a custom StatusCodeError class built in botbuilder, however we can't use it in the auth layers in botframework-connector
    • This dependency would break browser compatibility and would be a circular dependency
    • Alternatively moving into botframework-schema breaks backwards compatibility
  • Created AuthenticationError class (ctor needs status code and message) in botframework-connector
    • Auth layers that activate when BotFrameworkAdapter.authenticateConnection() is called now throw this custom error, so we rely less heavily on string-parsing to determine status code to throw. (Throwing errors w/status code already done in other parts of SDK as well):.
      • JwtTokenValidation
      • Channel Validation classes: ChannelValidation, EmulatorValidation, GovernmentChannelValidation, SkillValidation
      • JwtTokenExtractor
      • EndorsementsValidation
  • Made helper functions to determine auth code if the Adapter catches an error that does not have a status code, as fallback
    • (such as errors thrown from 3rd party libraries like jsonwebtoken that handles validating tokens with Channel Service's asymmetical signatures)
    • Note: TypeScript doesn't allow you to catch errors of a specific type 😭

Zerryth and others added 6 commits March 31, 2020 19:18
…erviceHandler.test.js because the unit tests are obscuring the correct expected result from botFrameworkAdapterStreaming.test.js
…asses tests; MockNetSocket now has createResponseFromError; JwtTokenValidation throws AuthenticationError instead of Error
…age; keep steven's conditional logic as fallback, in case we catch a non-StatusCodeError
@coveralls
Copy link

coveralls commented Apr 2, 2020

Pull Request Test Coverage Report for Build 123154

  • 72 of 90 (80.0%) changed or added relevant lines in 13 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 77.544%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botframework-connector/src/auth/channelValidation.ts 5 6 83.33%
libraries/botframework-connector/src/auth/enterpriseChannelValidation.ts 6 7 85.71%
libraries/botbuilder/src/botFrameworkAdapter.ts 6 8 75.0%
libraries/botframework-connector/src/auth/governmentChannelValidation.ts 5 7 71.43%
libraries/botframework-connector/src/auth/openIdMetadata.ts 2 4 50.0%
libraries/botframework-connector/src/auth/jwtTokenExtractor.ts 2 6 33.33%
libraries/botframework-connector/src/auth/emulatorValidation.ts 3 9 33.33%
Files with Coverage Reduction New Missed Lines %
libraries/botframework-streaming/src/webSocket/webSocketTransport.ts 2 80.65%
libraries/botframework-streaming/src/webSocket/webSocketServer.ts 7 66.67%
Totals Coverage Status
Change from base Build 123152: -0.03%
Covered Lines: 11976
Relevant Lines: 14717

💛 - Coveralls

@Zerryth Zerryth requested a review from carlosscastro April 9, 2020 20:56
@Zerryth Zerryth marked this pull request as ready for review April 9, 2020 20:57
Copy link
Contributor

@Stevenic Stevenic left a comment

Choose a reason for hiding this comment

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

:shipit:

@Zerryth
Copy link
Contributor Author

Zerryth commented Apr 21, 2020

Hmm, chrome tests are breaking in the build, and I don't believe it has anything to do with the PR changes...reached out to Bruce regarding that aspect. (UPDATE: seems like browser test breaks are a known issue, with Steven on getting the fix)

Thanks @Stevenic for reviewing! (Woohoo a PR review finally 😄)
@carlosscastro Steve took a look at the PR, but said that it'd be best if you looked into too before merging though, please

@Zerryth
Copy link
Contributor Author

Zerryth commented Apr 22, 2020

@stevengum @carlosscastro

  • Changed status codes thrown with the AuthenticationErrors pointed out
  • AuthenticationError unit tests are now separated into authenticationError.test.js
  • Replaced inexplicit numbers in unit tests with StatusCodes enum values instead

LMK if anything else is needed 😀

@Zerryth Zerryth merged commit e0ed178 into master Apr 22, 2020
@Zerryth Zerryth deleted the Zerryth/authErrors branch April 22, 2020 17:02
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.

[bf-connector] Cleanup Errors in authentication code
5 participants