Skip to content
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.

Sign up for GitHub

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

Enables FxCop Analyzer for the Connector library #4110

Merged
merged 2 commits into from
Jun 17, 2020
Merged

Enables FxCop Analyzer for the Connector library #4110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
077de5b
Enables FxCop Analyzer for the Connector library and addressed violat…
gabog Jun 17, 2020
d07c4ea
Address Carlos feedback.
gabog Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion libraries/Microsoft.Bot.Connector/AttachmentsEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ public partial class Attachments
/// <param name="attachmentId">id of the attachment.</param>
/// <param name="viewId">default is "original".</param>
/// <returns>uri.</returns>
#pragma warning disable CA1055 // Uri return values should not be strings (we can't change this without breaking binary compat)
public string GetAttachmentUri(string attachmentId, string viewId = "original")
#pragma warning restore CA1055 // Uri return values should not be strings
{
if (attachmentId == null)
{
Expand All @@ -41,7 +43,7 @@ public string GetAttachmentUri(string attachmentId, string viewId = "original")

// Construct URL
var baseUrl = this.Client.BaseUri.AbsoluteUri;
var url = new Uri(new Uri(baseUrl + (baseUrl.EndsWith("/") ? string.Empty : "/")), "v3/attachments/{attachmentId}/views/{viewId}").ToString();
var url = new Uri(new Uri(baseUrl + (baseUrl.EndsWith("/", StringComparison.OrdinalIgnoreCase) ? string.Empty : "/", StringComparison.OrdinalIgnoreCase)), "v3/attachments/{attachmentId}/views/{viewId}").ToString();
url = url.Replace("{attachmentId}", Uri.EscapeDataString(attachmentId));
url = url.Replace("{viewId}", Uri.EscapeDataString(viewId));
return url;
Expand Down
199 changes: 97 additions & 102 deletions libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,110 +90,15 @@ public async Task<AuthenticationResult> GetTokenAsync(bool forceRefresh = false)

async Task<AuthenticatorResult> IAuthenticator.GetTokenAsync(bool forceRefresh)
{
var result = await GetTokenAsync(forceRefresh);
var result = await GetTokenAsync(forceRefresh).ConfigureAwait(false);
return new AuthenticatorResult()
{
AccessToken = result.AccessToken,
ExpiresOn = result.ExpiresOn
};
}

private async Task<AuthenticationResult> AcquireTokenAsync(bool forceRefresh = false)
{
bool acquired = false;

if (forceRefresh)
{
authContext.TokenCache.Clear();
}

try
{
// The ADAL client team recommends limiting concurrency of calls. When the Token is in cache there is never
// contention on this semaphore, but when tokens expire there is some. However, after measuring performance
// with and without the semaphore (and different configs for the semaphore), not limiting concurrency actually
// results in higher response times overall. Without the use of this semaphore calls to AcquireTokenAsync can take up
// to 5 seconds under high concurrency scenarios.
acquired = tokenRefreshSemaphore.Wait(SemaphoreTimeout);

// If we are allowed to enter the semaphore, acquire the token.
if (acquired)
{
// Acquire token async using MSAL.NET
// https://github.com/AzureAD/azure-activedirectory-library-for-dotnet
// Given that this is a ClientCredential scenario, it will use the cache without the
// need to call AcquireTokenSilentAsync (which is only for user credentials).
// Scenario details: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Client-credential-flows#it-uses-the-application-token-cache
AuthenticationResult authResult = null;

// Password based auth
if (clientCredential != null)
{
authResult = await authContext.AcquireTokenAsync(authConfig.Scope, this.clientCredential).ConfigureAwait(false);
}

// Certificate based auth
else if (clientCertificate != null)
{
authResult = await authContext.AcquireTokenAsync(authConfig.Scope, clientCertificate, sendX5c: this.clientCertSendX5c).ConfigureAwait(false);
}

// This means we acquired a valid token successfully. We can make our retry policy null.
// Note that the retry policy is set under the semaphore so no additional synchronization is needed.
if (currentRetryPolicy != null)
{
currentRetryPolicy = null;
}

return authResult;
}
else
{
// If the token is taken, it means that one thread is trying to acquire a token from the server.
// If we already received information about how much to throttle, it will be in the currentRetryPolicy.
// Use that to inform our next delay before trying.
throw new ThrottleException() { RetryParams = currentRetryPolicy };
}
}
catch (Exception ex)
{
// If we are getting throttled, we set the retry policy according to the RetryAfter headers
// that we receive from the auth server.
// Note that the retry policy is set under the semaphore so no additional synchronization is needed.
if (IsAdalServiceUnavailable(ex))
{
currentRetryPolicy = ComputeAdalRetry(ex);
}

throw;
}
finally
{
// Always release the semaphore if we acquired it.
if (acquired)
{
ReleaseSemaphore();
}
}
}

private void ReleaseSemaphore()
{
try
{
tokenRefreshSemaphore.Release();
}
catch (SemaphoreFullException)
{
// We should not be hitting this after switching to SemaphoreSlim, but if we do hit it everything will keep working.
// Logging to have clear knowledge of whether this is happening.
logger?.LogWarning("Attempted to release a full semaphore.");
}

// Any exception other than SemaphoreFullException should be thrown right away
}

private RetryParams HandleAdalException(Exception ex, int currentRetryCount)
private static RetryParams HandleAdalException(Exception ex, int currentRetryCount)
{
if (IsAdalServiceUnavailable(ex))
{
Expand All @@ -220,7 +125,7 @@ private RetryParams HandleAdalException(Exception ex, int currentRetryCount)
}
}

private bool IsAdalServiceUnavailable(Exception ex)
private static bool IsAdalServiceUnavailable(Exception ex)
{
AdalServiceException adalServiceException = ex as AdalServiceException;
if (adalServiceException == null)
Expand All @@ -238,7 +143,7 @@ private bool IsAdalServiceUnavailable(Exception ex)
/// </summary>
/// <param name="ex">Exception.</param>
/// <returns>True if the exception represents an invalid request.</returns>
private bool IsAdalServiceInvalidRequest(Exception ex)
private static bool IsAdalServiceInvalidRequest(Exception ex)
{
if (ex is AdalServiceException adal)
{
Expand All @@ -253,7 +158,7 @@ private bool IsAdalServiceInvalidRequest(Exception ex)
return false;
}

private RetryParams ComputeAdalRetry(Exception ex)
private static RetryParams ComputeAdalRetry(Exception ex)
{
if (ex is AdalServiceException)
{
Expand Down Expand Up @@ -282,6 +187,22 @@ private RetryParams ComputeAdalRetry(Exception ex)

return RetryParams.DefaultBackOff(0);
}

private void ReleaseSemaphore()
{
try
{
tokenRefreshSemaphore.Release();
}
catch (SemaphoreFullException)
{
// We should not be hitting this after switching to SemaphoreSlim, but if we do hit it everything will keep working.
// Logging to have clear knowledge of whether this is happening.
logger?.LogWarning("Attempted to release a full semaphore.");
}

// Any exception other than SemaphoreFullException should be thrown right away
}

private void Initialize(OAuthConfiguration configurationOAuth, HttpClient customHttpClient)
{
Expand All @@ -296,9 +217,83 @@ private void Initialize(OAuthConfiguration configurationOAuth, HttpClient custom
}
}

private bool UseCertificate()
private async Task<AuthenticationResult> AcquireTokenAsync(bool forceRefresh = false)
{
return this.clientCertificate != null;
bool acquired = false;

if (forceRefresh)
{
authContext.TokenCache.Clear();
}

try
{
// The ADAL client team recommends limiting concurrency of calls. When the Token is in cache there is never
// contention on this semaphore, but when tokens expire there is some. However, after measuring performance
// with and without the semaphore (and different configs for the semaphore), not limiting concurrency actually
// results in higher response times overall. Without the use of this semaphore calls to AcquireTokenAsync can take up
// to 5 seconds under high concurrency scenarios.
acquired = tokenRefreshSemaphore.Wait(SemaphoreTimeout);

// If we are allowed to enter the semaphore, acquire the token.
if (acquired)
{
// Acquire token async using MSAL.NET
// https://github.com/AzureAD/azure-activedirectory-library-for-dotnet
// Given that this is a ClientCredential scenario, it will use the cache without the
// need to call AcquireTokenSilentAsync (which is only for user credentials).
// Scenario details: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Client-credential-flows#it-uses-the-application-token-cache
AuthenticationResult authResult = null;

// Password based auth
if (clientCredential != null)
{
authResult = await authContext.AcquireTokenAsync(authConfig.Scope, this.clientCredential).ConfigureAwait(false);
}

// Certificate based auth
else if (clientCertificate != null)
{
authResult = await authContext.AcquireTokenAsync(authConfig.Scope, clientCertificate, sendX5c: this.clientCertSendX5c).ConfigureAwait(false);
}

// This means we acquired a valid token successfully. We can make our retry policy null.
// Note that the retry policy is set under the semaphore so no additional synchronization is needed.
if (currentRetryPolicy != null)
{
currentRetryPolicy = null;
}

return authResult;
}
else
{
// If the token is taken, it means that one thread is trying to acquire a token from the server.
// If we already received information about how much to throttle, it will be in the currentRetryPolicy.
// Use that to inform our next delay before trying.
throw new ThrottleException() { RetryParams = currentRetryPolicy };
}
}
catch (Exception ex)
{
// If we are getting throttled, we set the retry policy according to the RetryAfter headers
// that we receive from the auth server.
// Note that the retry policy is set under the semaphore so no additional synchronization is needed.
if (IsAdalServiceUnavailable(ex))
{
currentRetryPolicy = ComputeAdalRetry(ex);
}

throw;
}
finally
{
// Always release the semaphore if we acquired it.
if (acquired)
{
ReleaseSemaphore();
}
}
}
}
}
11 changes: 6 additions & 5 deletions libraries/Microsoft.Bot.Connector/Authentication/AppCredentials.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Threading;
Expand All @@ -29,7 +30,7 @@ public abstract class AppCredentials : ServiceClientCredentials
/// <summary>
/// Authenticator abstraction used to obtain tokens through the Client Credentials OAuth 2.0 flow.
/// </summary>
private readonly Lazy<IAuthenticator> authenticator;
private Lazy<IAuthenticator> authenticator;

/// <summary>
/// Initializes a new instance of the <see cref="AppCredentials"/> class.
Expand All @@ -52,7 +53,6 @@ public AppCredentials(string channelAuthTenant = null, HttpClient customHttpClie
public AppCredentials(string channelAuthTenant = null, HttpClient customHttpClient = null, ILogger logger = null, string oAuthScope = null)
{
OAuthScope = string.IsNullOrWhiteSpace(oAuthScope) ? AuthenticationConstants.ToChannelFromBotOAuthScope : oAuthScope;
authenticator = BuildIAuthenticator();
ChannelAuthTenant = channelAuthTenant;
CustomHttpClient = customHttpClient;
Logger = logger;
Expand Down Expand Up @@ -84,7 +84,7 @@ public string ChannelAuthTenant
set
{
// Advanced user only, see https://aka.ms/bots/tenant-restriction
var endpointUrl = string.Format(AuthenticationConstants.ToChannelFromBotLoginUrlTemplate, value);
var endpointUrl = string.Format(CultureInfo.InvariantCulture, AuthenticationConstants.ToChannelFromBotLoginUrlTemplate, value);

if (Uri.TryCreate(endpointUrl, UriKind.Absolute, out var result))
{
Expand All @@ -103,7 +103,7 @@ public string ChannelAuthTenant
/// <value>
/// The OAuth endpoint to use.
/// </value>
public virtual string OAuthEndpoint => string.Format(AuthenticationConstants.ToChannelFromBotLoginUrlTemplate, ChannelAuthTenant);
public virtual string OAuthEndpoint => string.Format(CultureInfo.InvariantCulture, AuthenticationConstants.ToChannelFromBotLoginUrlTemplate, ChannelAuthTenant);

/// <summary>
/// Gets the OAuth scope to use.
Expand Down Expand Up @@ -200,6 +200,7 @@ public override async Task ProcessHttpRequestAsync(HttpRequestMessage request, C
/// <remarks>If the task is successful, the result contains the access token string.</remarks>
public async Task<string> GetTokenAsync(bool forceRefresh = false)
{
authenticator ??= BuildIAuthenticator();
var token = await authenticator.Value.GetTokenAsync(forceRefresh).ConfigureAwait(false);
return token.AccessToken;
}
Expand Down Expand Up @@ -242,7 +243,7 @@ private static bool IsTrustedUrl(Uri uri)
}
}

private bool ShouldSetToken(HttpRequestMessage request)
private static bool ShouldSetToken(HttpRequestMessage request)
{
if (IsTrustedUrl(request.RequestUri))
{
Expand Down
6 changes: 5 additions & 1 deletion libraries/Microsoft.Bot.Connector/Authentication/AuthenticationConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;

namespace Microsoft.Bot.Connector.Authentication
{
/// <summary>
Expand All @@ -12,7 +14,9 @@ namespace Microsoft.Bot.Connector.Authentication
/// </remarks>
public class AuthenticationConfiguration
{
public string[] RequiredEndorsements { get; set; } = { };
#pragma warning disable CA1819 // Properties should not return arrays (we can't change this without breaking binary compat)
public string[] RequiredEndorsements { get; set; } = Array.Empty<string>();
#pragma warning restore CA1819 // Properties should not return arrays

/// <summary>
/// Gets or sets an <see cref="ClaimsValidator"/> instance used to validate the identity claims.
Expand Down
10 changes: 6 additions & 4 deletions libraries/Microsoft.Bot.Connector/Authentication/ChannelValidation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public static class ChannelValidation
/// <value>
/// The default endpoint that is used for Open ID Metadata requests.
/// </value>
#pragma warning disable CA1056 // Uri properties should not be strings (we can't change this without breaking binary compat)
public static string OpenIdMetadataUrl { get; set; } = AuthenticationConstants.ToBotFromChannelOpenIdMetadataUrl;
#pragma warning restore CA1056 // Uri properties should not be strings

/// <summary>
/// Validate the incoming Auth Header as a token sent from the Bot Framework Service.
Expand Down Expand Up @@ -86,7 +88,7 @@ public static async Task<ClaimsIdentity> AuthenticateChannelToken(string authHea
OpenIdMetadataUrl,
AuthenticationConstants.AllowedSigningAlgorithms);

var identity = await tokenExtractor.GetIdentityAsync(authHeader, channelId, authConfig.RequiredEndorsements);
var identity = await tokenExtractor.GetIdentityAsync(authHeader, channelId, authConfig.RequiredEndorsements).ConfigureAwait(false);
if (identity == null)
{
// No valid identity. Not Authorized.
Expand Down Expand Up @@ -123,7 +125,7 @@ public static async Task<ClaimsIdentity> AuthenticateChannelToken(string authHea
throw new UnauthorizedAccessException();
}

if (!await credentials.IsValidAppIdAsync(appIdFromClaim))
if (!await credentials.IsValidAppIdAsync(appIdFromClaim).ConfigureAwait(false))
{
// The AppId is not valid. Not Authorized.
throw new UnauthorizedAccessException($"Invalid AppId passed on token: {appIdFromClaim}");
Expand Down Expand Up @@ -167,7 +169,7 @@ public static async Task<ClaimsIdentity> AuthenticateChannelToken(string authHea
throw new ArgumentNullException(nameof(authConfig));
}

var identity = await AuthenticateChannelToken(authHeader, credentials, httpClient, channelId, authConfig);
var identity = await AuthenticateChannelToken(authHeader, credentials, httpClient, channelId, authConfig).ConfigureAwait(false);

var serviceUrlClaim = identity.Claims.FirstOrDefault(claim => claim.Type == AuthenticationConstants.ServiceUrlClaim)?.Value;
if (string.IsNullOrWhiteSpace(serviceUrlClaim))
Expand All @@ -176,7 +178,7 @@ public static async Task<ClaimsIdentity> AuthenticateChannelToken(string authHea
throw new UnauthorizedAccessException();
}

if (!string.Equals(serviceUrlClaim, serviceUrl))
if (!string.Equals(serviceUrlClaim, serviceUrl, StringComparison.OrdinalIgnoreCase))
{
// Claim must match. Not Authorized.
throw new UnauthorizedAccessException();
Expand Down
Loading