Certificate authentication: Enable cert auth and start the shift away from ICredentialProvider. #2524
Conversation
… from ICredentialProvider.
Pull Request Test Coverage Report for Build 79473
💛 - Coveralls |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
…icateAppCredentials
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
|
And just to set expectations, this change goes from Only App id + password can be done, to 'We natively support password or certificate authentication, but the design allows for simple extension to any other client flow supported by aad apps, only by adding a new AppCredentials subclass without any further modifications to existing abstractions.' |
|
Also, tests exist but not checked in, waiting for us to figure out some build details |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
| /// <param name="expirationTime">The expiration time after which this service url is not trusted anymore.</param> | ||
| public static void TrustServiceUrl(string serviceUrl, DateTime expirationTime) | ||
| { | ||
| lock (TrustedHostNames) |
There was a problem hiding this comment.
if the dictionary TrustedHostNames were a Concurrent Dictionary, could we eliminate these locks?
| if (TrustedHostNames.TryGetValue(uri.Host, out DateTime trustedServiceUrlExpiration)) | ||
| { | ||
| // check if the trusted service url is still valid | ||
| if (trustedServiceUrlExpiration > DateTime.UtcNow.Subtract(TimeSpan.FromMinutes(5))) |
There was a problem hiding this comment.
I'm not sure why "5 minutes" is a magic number. Wouldn't it simply be >= for the true/false check?
| @@ -19,7 +19,7 @@ namespace Microsoft.Bot.Connector.Authentication | |||
| /// <summary> | |||
| /// MicrosoftAppCredentials auth implementation and cache. | |||
| /// </summary> | |||
| public class MicrosoftAppCredentials : ServiceClientCredentials | |||
| public class MicrosoftAppCredentials : AppCredentials | |||
There was a problem hiding this comment.
From an OO perspective, this is certainly legit. I think it as as well from a .NET compat perspetive.
|
This dropped code coverage by 3%. Can you please add relevant tests to get back to the basleine number? |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
Enable cert-based authentication of AAD apps. Currently we only support app id + password, but AAD also supports certificates:
To enable this, we start some key refactors after identifying shortcomings of the current model. ICredentialProvider has 2 problems:
To overcome this, this is the small first step towards using AppCredentials for AAD verifications, which currently supports Certs or Password, but as-is can simply be extended to support any of the 7 ADAL client flows.
The default paths still use ICredentialProvider and we are enabling AppCredentials for new scenarios. A second iteration will make AppCredentials be the default, but we want to stage that transition to minimize risk.