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: Add Audience for Certificate auth to work with Skills #6794

Merged
merged 2 commits into from
May 28, 2024

Conversation

sw-joelmut
Copy link
Collaborator

#minor

Description

This PR adds Audience to CertificateAppCredentials, so it is passed correctly when calling a Skill bot. Additionally, these changes take into consideration the PR #6694, so the improvement isn't lost in the process.

Specific Changes

  • Added missing oAuthScope parameter to libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs.
  • Improved libraries/Microsoft.Bot.Connector/Authentication/CertificateServiceClientCredentialsFactory.cs by reverting the changes made in the PR Support SN+I authentication with AAD #6694, but maintaining the improvement.
    • It creates a new instance of CertificateAppCredentials per Audience, instead of having just one with the default api.botframework.
  • Added unit test to confirm it doesn't create new instances when the same Audience is provided.

Testing

The following images show the new unit test passing and the Root + Skill bots working as expected.
image
image

@sw-joelmut sw-joelmut added the Automation: No parity PR does not need to be applied to other languages. label May 27, 2024
@sw-joelmut sw-joelmut requested a review from tracyboehrer May 27, 2024 14:49
@sw-joelmut sw-joelmut requested a review from a team as a code owner May 27, 2024 14:49
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 389711

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.004%) to 78.171%

Files with Coverage Reduction New Missed Lines %
/libraries/AdaptiveExpressions/BuiltinFunctions/GetPreviousViableTime.cs 1 90.91%
/libraries/AdaptiveExpressions/BuiltinFunctions/GetNextViableTime.cs 1 93.94%
/libraries/AdaptiveExpressions/LRUCache.cs 4 87.18%
/libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs 16 17.07%
Totals Coverage Status
Change from base Build 389234: -0.004%
Covered Lines: 26192
Relevant Lines: 33506

💛 - Coveralls

@BruceHaley
Copy link
Contributor

✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll

@tracyboehrer tracyboehrer merged commit 259c174 into main May 28, 2024
15 checks passed
@tracyboehrer tracyboehrer deleted the southworks/fix/skills-with-certificates branch May 28, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation: No parity PR does not need to be applied to other languages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants