Skills use the right connector client when receiving tokens from TokenService #3822
Conversation
…ce response + Introduce IConnectorClientProvider to expose Adapter Functionality to OAuthPrompt (this layering issue to be addressed in R10) + Move IExtendedUserTokenProvider and IUserTokenProvider to the Oauth folder to avoid polluting the builder directory (cannot change the namespace for backward compat unfortunately) + Oauthprompt persists caller info to interpret callbacks from token service appropritately and build the right connector client following
libraries/Microsoft.Bot.Builder/OAuth/IConnectorClientProvider.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Abstraction to build connector clients. | ||
| /// </summary> | ||
| public interface IConnectorClientProvider |
There was a problem hiding this comment.
If we refactor the adapter and auth layers in R10, should this interface be here or in the Connector dll?
There was a problem hiding this comment.
Decided offline that @carlosscastro going to rename the interface and keep it in this DLL as it's for concealing a higher-level implementation.
Additionally, the ServiceUrl, ClaimsIdentity and Audience are not all the requisite pieces for making a ConnectorClient, an IServiceClientCredentialProvider is required as well.
The port to JS and Python is uncertain as createConnectorClient is a public method in those languages.
There was a problem hiding this comment.
Since nothing is really "private" or "protected" in Python, it should be a minimal concern. i.e., If it had been notated as private, people can still access and override it. It's more of a pirate guideline. Meant to be broken if you feel like it. Like types.
There was a problem hiding this comment.
With that said, not sure I see a problem anyway. It appears that now this method is declared in an interface, and calls the internal private version. But it has the same signature. So really aren't we just declaring it in an abstract class, and overriding it with the current impl (for Python)?
There was a problem hiding this comment.
Yes, forget about the explicit implementatin (which is C# sugar). The goal of this is that the OAuthPrompt deals with an IConnectorClientBuilder rather than an adapter, which gives us the possibility of decoupling from the adapter. Particularly considering that we want to do serious changes in the adapter and auth layer :)
There was a problem hiding this comment.
IMHO, between this, PR #3818 and the large amount of code in the BotFrameworkAdapter, there is definitely a decent amount of work for comparable value in decoupling.
There was a problem hiding this comment.
We want to get to a point where all those 1000 lines of auth prompt code are completely outside the adapter. We'll get there.
Co-Authored-By: Steven Gum <14935595+stevengum@users.noreply.github.com>
…builder-dotnet into ccastro/skillClaim
carlosscastro
requested review from
gabog,
johnataylor and
mrivera-ms
as code owners
carlosscastro
changed the title
This PR contains Jeff's changes that allow skills to correctly interpret responses from the token service and create the right connector client. I added some surface changes.
Note that there is a general layering problematic which is, the OAuthPrompt (in Dialogs package) is the only dialog that needs to go back and invoke stuff in the Adapter layer. Because of this, there is a lot of code in the adapter that needs to support that. Refactoring of the adapter and auth layer in R10 should include general solutions for this layering problematic that grew organically.
This burden is now in the sample and taking this code, the sample can be super simple and support oauth seamlessly.
Fixes #3617