-
Notifications
You must be signed in to change notification settings - Fork 283
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: create conversation #3805
Conversation
836f893
to
b223eff
Compare
Pull Request Test Coverage Report for Build 969180472
💛 - Coveralls |
Note: *Async postfix is due to conflicts with BotFrameworkAdapter Fixes #3752
b223eff
to
1c3e9d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the applicable Teams flows functional with these changes?
// Create a turn context and run the pipeline. | ||
const context = this.createTurnContext( | ||
createActivity, | ||
claimsIdentity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that at this point, the claimsIdentity
would have an audienceClaim with the value of audience
, not the botAppId
:
botbuilder-js/libraries/botbuilder-core/src/cloudAdapterBase.ts
Lines 307 to 315 in 0db8231
return new ClaimsIdentity([ | |
{ | |
type: AuthenticationConstants.AudienceClaim, | |
value: botAppId, | |
}, | |
{ | |
type: AuthenticationConstants.AppIdClaim, | |
value: botAppId, | |
}, |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Working on testing in Teams. EDIT: @stevengum, I was able to successfully call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, let's follow up on the proper claims construction in the .NET repo
Fixes #3752