botframework-sdk icon indicating copy to clipboard operation
botframework-sdk copied to clipboard

CloudAdapterBase.createConversation() creates inaccurate ClaimsIdentity

Open stevengum opened this issue 4 years ago • 0 comments

Version

.NET: 4.13.3 of Microsoft.Bot.Builder JS: Upcoming R14 release (https://github.com/microsoft/botbuilder-js/commit/18b3ebb175ab03b0754fb7a5d6993519e350b859) Python: Upcoming R14 release Java: Upcoming R14 release

Describe the bug

The created ClaimsIdentity set on the TurnState in the CloudAdapterBase.createConversation*() scenarios is incomplete by virtue of not having the correct Audience claim set. The ConnectorClient on the TurnState will have the correct audience set for the SDK's BotFrameworkAuthentication implementations but logic relying on the TurnState-cached ClaimsIdentity will see the following claims:

  • AppId Claim == botAppId argument
  • Audience Claim == botAppId argument // should be audience argument
  • AppId Claim == serviceUrl argument

Additional context:

JS discussion https://github.com/microsoft/botbuilder-js/pull/3805#discussion_r658226874

I would expect that at this point, the claimsIdentity would have an audienceClaim with the value of audience, not the botAppId:

https://github.com/microsoft/botbuilder-js/blob/0db823143a83267c60d42a7daef2cb04cc72ee4a/libraries/botbuilder-core/src/cloudAdapterBase.ts#L307-L315

Is there a reason this isn't the case?

What will happen is that the ClaimsIdentity is essentially Emulator-esque as the appId and audience claim are the same value (the botAppId), whereas the created ConnectorClient is properly addressed to the passed in audience. This audience will can be a channel, another bot, or something else entirely.

Originally posted by @stevengum in https://github.com/microsoft/botbuilder-js/pull/3805#discussion_r658226874

Not sure, I followed John's lead in porting this. As far as I can tell, this is equivalent to .NET. Perhaps we should file an issue on .NET?

Originally posted by @joshgummersall in https://github.com/microsoft/botbuilder-js/pull/3805#discussion_r658235375

Expected behavior

The unfortunate pattern of consuming data from the ClaimsIdentity via the TurnState is one occasionally found in the SDK and likely in customers' bots. The ClaimsIdentity and ConnectorClient on the TurnState should accurately reflect each other.

FYI @johnataylor @joshgummersall @mrivera-ms @LeeParrishMSFT @axelsrz @batta32

Tracking Status

Dotnet SDK TODO

  • [ ] PR
  • [ ] Merged

Javascript SDK TODO

  • [ ] PR
  • [ ] Merged

Python SDK TODO

  • [ ] PR
  • [ ] Merged

Java SDK TODO

  • [ ] PR
  • [ ] Merged

stevengum avatar Jun 25 '21 00:06 stevengum