Adding ClaimsIdentity and ConnectorClient to the services collection - #356
Conversation
…and removing BotFrameworkTurnContext.
drub0y
left a comment
drub0y
left a comment
There was a problem hiding this comment.
In general, I like this approach a lot. I especially 💗 the connector client being moved into the services collection. It was on my list since we already knew it was terribad to be spinning up a new instance on every outbound activity and this addresses that.
| new Claim(AuthenticationConstants.AppIdClaim, botAppId) | ||
| }); | ||
|
|
||
| context.Services.Add<IIdentity>(claimsIdentity); |
There was a problem hiding this comment.
Is there really any value adding this to the services? You directly pass it to CreateConnectorClientAsync, so it's not like there's any code that's looking for it.
If you still think it should be registered, might I suggest you register it under an explicit, well known key (maybe something like "BotIdentity") rather than as "the" IIdentity since one could imagine there being some conflict around that interface at some point.
There was a problem hiding this comment.
This is for extension SDKs to use. If you want to build a middleware for creating custom connector client (Teams extension SDK) or for creating other clients (e.g. Graph client or something).
There was a problem hiding this comment.
Will put it behind a key though.
| string botAppId = GetBotId(claimsIdentity); | ||
| var context = new BotFrameworkTurnContext(botAppId, this, activity); | ||
| var context = new TurnContext(this, activity); | ||
| context.Services.Add<IIdentity>(claimsIdentity); |
There was a problem hiding this comment.
Same comment as above. Registration of this seems unnecessary, but if you do it consider using the same explicit key approach mentioned above.
| var context = new BotFrameworkTurnContext(botAppId, this, activity); | ||
| var context = new TurnContext(this, activity); | ||
| context.Services.Add<IIdentity>(claimsIdentity); | ||
| IConnectorClient connectorClient = await this.CreateConnectorClientAsync(claimsIdentity, activity.ServiceUrl); |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
| }); | ||
|
|
||
| context.Services.Add<IIdentity>(claimsIdentity); | ||
| IConnectorClient connectorClient = await this.CreateConnectorClientAsync(claimsIdentity, reference.ServiceUrl); |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
| var context = new TurnContext(this, reference.GetPostToBotMessage()); | ||
|
|
||
| // Hand craft Claims Identity. | ||
| ClaimsIdentity claimsIdentity = new ClaimsIdentity(new List<Claim> |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
| Claim botAppIdClaim = (claimsIdentity.Claims?.SingleOrDefault(claim => claim.Type == AuthenticationConstants.AudienceClaim) | ||
| // For requests from channel App Id is in Audience claim of JWT token. For emulator it is in AppId claim. For | ||
| // unauthenticated requests we have anonymouse identity provided auth is disabled. | ||
| Claim botAppIdClaim = (claimsIdentity.Claims?.SingleOrDefault(claim => claim.Type == AuthenticationConstants.AudienceClaim) |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
| { | ||
| return botAppIdClaim.Value; | ||
| string botId = botAppIdClaim.Value; | ||
| MicrosoftAppCredentials appCredentials = await this.GetAppCredentialsAsync(botId); |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
|
|
||
| if (!_appCredentialMap.TryGetValue(appId, out var appCredentials)) | ||
| { | ||
| string appPassword = await _credentialProvider.GetAppPasswordAsync(appId); |
There was a problem hiding this comment.
We should be consistent in our use of var vs explicit types. Right now the codebase follows a var-only approach so, religious debates aside, I think we should stick with that.
|
@drub0y I think I have addressed all the feedback. Let me know if you have any more concerns. |
drub0y
left a comment
drub0y
left a comment
There was a problem hiding this comment.
I would remove adding the IIdentity from the services since nothing uses it (more details inline), other than that I think this is good to go! If you have a reason to keep it, just lemme know, otherwise remove and I'm good to approve after that.
|
|
||
| context.Services.Add<IIdentity>(claimsIdentity); | ||
| IConnectorClient connectorClient = await this.CreateConnectorClientAsync(claimsIdentity, reference.ServiceUrl); | ||
| context.Services.Add<IIdentity>("BotIdentity", claimsIdentity); |
There was a problem hiding this comment.
So I guess I still question ever putting this into the services collection seeing as how you're already handing it directly to CreateConnectorClientAsync and using it directly there. No code ever goes back and looks for it in the Services collection. I would suggest we avoid putting extra "stuff" into the Services collection unless we're actually going to need to pull it out from there later and, as I said, this code doesn't ever do that... it only ever goes and pulls the IConnectorClient back out.
Given that, I would suggest removing this from the services collection until we actually deem it necessary.
There was a problem hiding this comment.
IIDentity is added so Extension SDKs like Teams can access the Bot AppId and then create their own ConnectorClients(or other clients). Plan is with access to BotAppId and ICredentialProvider they can build their own clients.
| var context = new TurnContext(this, activity); | ||
| context.Services.Add<IIdentity>(claimsIdentity); | ||
| IConnectorClient connectorClient = await this.CreateConnectorClientAsync(claimsIdentity, activity.ServiceUrl); | ||
| context.Services.Add<IIdentity>("BotIdentity", claimsIdentity); |
There was a problem hiding this comment.
IIDentity is added so Extension SDKs like Teams can access the Bot AppId and then create their own ConnectorClients(or other clients). Plan is with access to BotAppId and ICredentialProvider they can build their own clients.
* Initial template for echo bot * Added template zip file * Added template zip file * Readme for top level generators folder * Pylint fixes for generator, readme updates * Adding empty bot generator, ported latest on_error changes * PR changes addressed
2 participants
Adding ClaimsIdentity and ConnectorClient to the services collection and removing BotFrameworkTurnContext.
Why? - This means extensions like Teams no longer have to explicitly depend on BotFrameworkAdapter but instead can depend on presence of IIdentity for getting BotAppId and IConnectorClient to get ConnectorClient in the Middleware. Or new clients can be created using ICredentialProvider + IIdentity. Since both IIdentity and IConnectorClient are outside BotFrameworkAdapter the dependency is optional now.
Also this allows developers to call ConnectorClient methods directly which were otherwise hidden away.
What this does not solve? BotFrameworkAdapter is still special cased as it requires more info when compared to base BotAdapter and thus does not follow BotAdapter in few specific cases. Will try to fix this in a different PR (or atleast part of it).