Onboard Id Web to Threading Analyzers #3041
Conversation
There was a problem hiding this comment.
LGTM at first review, @westin-m
Make sure you run end to end tests with credentials.
There was a race condition at some point, which is why the credential loaders used GetAwaiter.GetAsync().
I'm surprised there is no change in the public API?
src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
I've added this to the description |
| /// </summary> | ||
| /// <param name="certificateDescriptions">Description of the certificates.</param> | ||
| /// <returns>First certificate in the certificate description list.</returns> | ||
| public static async Task<X509Certificate2?> LoadFirstCertificateAsync(IEnumerable<CertificateDescription> certificateDescriptions) |
There was a problem hiding this comment.
Not for this PR, but long-term we should add an overload that accepts a CancellationToken, which should be propagated down - LoadCredentialsIfNeededAsync uses a semaphore which accepts a cancellation token.
| { | ||
| LabResponse labResponse = await LabUserHelper.GetDefaultUserAsync(); | ||
|
|
||
| var clientEnvVars = new Dictionary<string, string>(); | ||
|
|
||
| await ExecuteWebAppCallsGraphFlow(labResponse.User.Upn, labResponse.User.GetOrFetchPassword(), clientEnvVars, TraceFileClassName); | ||
| await ExecuteWebAppCallsGraphFlowAsync(labResponse.User.Upn, labResponse.User.GetOrFetchPassword(), clientEnvVars, TraceFileClassName).ConfigureAwait(false); |
There was a problem hiding this comment.
Remove ConfigureAwait in xUnit tests for better performance.
| { | ||
| _synclock.EnterWriteLock(); | ||
| try | ||
| { | ||
| OpenIdConnectConfiguration config = Task.Run(_configManager.GetConfigurationAsync).Result; | ||
| OpenIdConnectConfiguration config = await _configManager.GetConfigurationAsync(); |
There was a problem hiding this comment.
Need to revert and block it here.
https://stackoverflow.com/questions/59048102/c-sharp-fire-and-forget-task-and-discard
The following E2E scenarios were tested using the corresponding devapps:
Web app calls web api call graph
B2C web app calls web api
Owin
Blazer server calls web api
Blazer server B2C calls api
Ciam web app calls api
Web app calls graph
Deamon console calling downstream api, graph
All are working as expected.