Skip to content

remove trusted service url check in appcredentials #4827

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

Merged
merged 3 commits into from
Oct 22, 2020
Merged

remove trusted service url check in appcredentials #4827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4d241e
remove trusted service url check in appcredentials
johnataylor Oct 20, 2020
d2b34c9
cr feedback
johnataylor Oct 21, 2020
54ed0c5
cr feedback
johnataylor Oct 21, 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
52 changes: 11 additions & 41 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 @@ -19,6 +19,7 @@ namespace Microsoft.Bot.Connector.Authentication
/// </summary>
public abstract class AppCredentials : ServiceClientCredentials
{
[Obsolete]
internal static readonly IDictionary<string, DateTime> TrustedHostNames = new Dictionary<string, DateTime>
{
// { "state.botframework.com", DateTime.MaxValue }, // deprecated state api
Expand Down Expand Up @@ -145,37 +146,33 @@ public string ChannelAuthTenant
/// </summary>
/// <param name="serviceUrl">The service URL.</param>
/// <remarks>If expiration time is not provided, the expiration time will DateTime.UtcNow.AddDays(1).</remarks>
#pragma warning disable CA1801 // Review unused parameters
public static void TrustServiceUrl(string serviceUrl)
#pragma warning restore CA1801 // Review unused parameters
{
TrustServiceUrl(serviceUrl, DateTime.UtcNow.Add(TimeSpan.FromDays(1)));
}

/// <summary>
/// Adds the host of service url to <see cref="MicrosoftAppCredentials"/> trusted hosts.
/// </summary>
/// <param name="serviceUrl">The service URL.</param>
/// <param name="expirationTime">The expiration time after which this service url is not trusted anymore.</param>
#pragma warning disable CA1801 // Review unused parameters
public static void TrustServiceUrl(string serviceUrl, DateTime expirationTime)
#pragma warning restore CA1801 // Review unused parameters
{
lock (TrustedHostNames)
{
TrustedHostNames[new Uri(serviceUrl).Host] = expirationTime;
}
}

/// <summary>
/// Checks if the service url is for a trusted host or not.
/// </summary>
/// <param name="serviceUrl">The service url.</param>
/// <returns>True if the host of the service url is trusted; False otherwise.</returns>
#pragma warning disable CA1801 // Review unused parameters
public static bool IsTrustedServiceUrl(string serviceUrl)
#pragma warning restore CA1801 // Review unused parameters
{
if (Uri.TryCreate(serviceUrl, UriKind.Absolute, out var uri))
{
return IsTrustedUrl(uri, NullLogger.Instance);
}

return false;
return true;
}

/// <summary>
Expand All @@ -185,7 +182,7 @@ public static bool IsTrustedServiceUrl(string serviceUrl)
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public override async Task ProcessHttpRequestAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
if (ShouldSetToken(request, Logger))
if (ShouldSetToken())
{
var token = await GetTokenAsync().ConfigureAwait(false);
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token);
Expand Down Expand Up @@ -234,42 +231,15 @@ protected virtual Lazy<IAuthenticator> BuildIAuthenticator()
LazyThreadSafetyMode.ExecutionAndPublication);
}

private static bool IsTrustedUrl(Uri uri, ILogger logger)
{
lock (TrustedHostNames)
{
if (TrustedHostNames.TryGetValue(uri.Host, out var trustedServiceUrlExpiration))
{
// check if the trusted service url is still valid
if (trustedServiceUrlExpiration > DateTime.UtcNow.Subtract(TimeSpan.FromMinutes(5)))
{
return true;
}

logger.LogWarning($"{typeof(AppCredentials).FullName}.IsTrustedUrl(): '{uri}' found in TrustedHostNames but it expired (Expiration is set to: {trustedServiceUrlExpiration}, current time is {DateTime.UtcNow}).");
return false;
}

logger.LogWarning($"{typeof(AppCredentials).FullName}.IsTrustedUrl(): '{uri}' not found in TrustedHostNames.");
return false;
}
}

private bool ShouldSetToken(HttpRequestMessage request, ILogger logger)
private bool ShouldSetToken()
{
if (string.IsNullOrEmpty(MicrosoftAppId) || MicrosoftAppId == AuthenticationConstants.AnonymousSkillAppId)
{
// We don't set the token if the AppId is not set, since it means that we are in an un-authenticated scenario
return false;
}

if (IsTrustedUrl(request.RequestUri, logger))
{
return true;
}

logger.LogWarning($"{typeof(AppCredentials).FullName}.ShouldSetToken(): '{request.RequestUri.Authority}' is not trusted and JwtToken cannot be sent to it.");
return false;
return true;
}
}
}
12 changes: 0 additions & 12 deletions tests/Microsoft.Bot.Builder.Tests/BotFrameworkAdapterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,18 +305,12 @@ public async Task ContinueConversationAsyncWithoutAudience()
var scope = turnContext.TurnState.Get<string>(BotAdapter.OAuthScopeKey);
Assert.Equal(AuthenticationConstants.ToChannelFromBotOAuthScope, scope);

// Ensure the serviceUrl was added to the trusted hosts
Assert.True(AppCredentials.TrustedHostNames.ContainsKey(new Uri(channelServiceUrl).Host));

return Task.CompletedTask;
});

// Create ConversationReference to send a proactive message from Skill1 to a channel
var refs = new ConversationReference(serviceUrl: channelServiceUrl);

// Ensure the serviceUrl is NOT in the trusted hosts
Assert.False(AppCredentials.TrustedHostNames.ContainsKey(new Uri(channelServiceUrl).Host));

await adapter.ContinueConversationAsync(skillsIdentity, refs, callback, default);
}

Expand Down Expand Up @@ -368,18 +362,12 @@ public async Task ContinueConversationAsyncWithAudience()
var scope = turnContext.TurnState.Get<string>(BotAdapter.OAuthScopeKey);
Assert.Equal(skill2AppId, scope);

// Ensure the serviceUrl was added to the trusted hosts
Assert.True(AppCredentials.TrustedHostNames.ContainsKey(new Uri(skill2ServiceUrl).Host));

return Task.CompletedTask;
});

// Create ConversationReference to send a proactive message from Skill1 to Skill2
var refs = new ConversationReference(serviceUrl: skill2ServiceUrl);

// Ensure the serviceUrl is NOT in the trusted hosts
Assert.False(AppCredentials.TrustedHostNames.ContainsKey(new Uri(skill2ServiceUrl).Host));

await adapter.ContinueConversationAsync(skillsIdentity, refs, skill2AppId, callback, default);
}

Expand Down
9 changes: 5 additions & 4 deletions tests/Microsoft.Bot.Builder.Tests/Teams/TeamsInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ public async Task TestSendMessageToTeamsChannelAsync()

// Set a special base address so then we can make sure the connector client is honoring this http client
customHttpClient.BaseAddress = baseUri;
var connectorClient = new ConnectorClient(new Uri("https://test.coffee"), new MicrosoftAppCredentials("big-guid-here", "appPasswordHere"), customHttpClient);

var connectorClient = new ConnectorClient(new Uri("https://test.coffee"), MicrosoftAppCredentials.Empty, customHttpClient);

var activity = new Activity
{
Type = "message",
Expand All @@ -42,8 +43,8 @@ public async Task TestSendMessageToTeamsChannelAsync()
},
},
};
var turnContext = new TurnContext(new BotFrameworkAdapter(new SimpleCredentialProvider("big-guid-here", "appPasswordHere"), customHttpClient: customHttpClient), activity);

var turnContext = new TurnContext(new BotFrameworkAdapter(new SimpleCredentialProvider(), customHttpClient: customHttpClient), activity);
turnContext.TurnState.Add<IConnectorClient>(connectorClient);
turnContext.Activity.ServiceUrl = "https://test.coffee";
var handler = new TestTeamsActivityHandler();
Expand Down Expand Up @@ -301,7 +302,7 @@ private async Task CallSendMessageToTeamsChannelAsync(ITurnContext turnContext)
{
var message = MessageFactory.Text("hi");
var channelId = "channelId123";
var creds = new MicrosoftAppCredentials("big-guid-here", "appPasswordHere");
var creds = new MicrosoftAppCredentials(string.Empty, string.Empty);
var cancelToken = new CancellationToken();
var reference = await TeamsInfo.SendMessageToTeamsChannelAsync(turnContext, message, channelId, creds, cancelToken);

Expand Down
39 changes: 0 additions & 39 deletions tests/Microsoft.Bot.Connector.Tests/Authentication/JwtTokenValidationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,26 +109,6 @@ await JwtTokenValidation.AuthenticateRequest(
Assert.True(AppCredentials.IsTrustedServiceUrl("https://smba.trafficmanager.net/amer-client-ss.msg/"));
}

/// <summary>
/// Tests with a valid Token and invalid service url; and ensures that Service url is NOT added to Trusted service url list.
/// </summary>
[Fact]
public async void Channel_MsaHeader_Invalid_ServiceUrlShouldNotBeTrusted()
{
string header = $"Bearer {await new MicrosoftAppCredentials("2cd87869-38a0-4182-9251-d056e8f0ac24", "2.30Vs3VQLKt974F").GetTokenAsync()}";
var credentials = new SimpleCredentialProvider("7f74513e-6f96-4dbc-be9d-9a81fea22b88", string.Empty);

await Assert.ThrowsAsync<UnauthorizedAccessException>(
async () => await JwtTokenValidation.AuthenticateRequest(
new Activity { ServiceUrl = "https://webchat.botframework.com/" },
header,
credentials,
new SimpleChannelProvider(),
emptyClient));

Assert.False(MicrosoftAppCredentials.IsTrustedServiceUrl("https://webchat.botframework.com/"));
}

/// <summary>
/// Tests with no authentication header and makes sure the service URL is not added to the trusted list.
/// </summary>
Expand Down Expand Up @@ -174,25 +154,6 @@ public async void Channel_AuthenticationDisabledAndSkill_ShouldBeAnonymous()
Assert.Equal(AuthenticationConstants.AnonymousSkillAppId, JwtTokenValidation.GetAppIdFromClaims(claimsPrincipal.Claims));
}

/// <summary>
/// Tests with no authentication header and makes sure the service URL is not added to the trusted list.
/// </summary>
[Fact]
public async void Channel_AuthenticationDisabled_ServiceUrlShouldNotBeTrusted()
{
var header = string.Empty;
var credentials = new SimpleCredentialProvider();

var claimsPrincipal = await JwtTokenValidation.AuthenticateRequest(
new Activity { ServiceUrl = "https://webchat.botframework.com/" },
header,
credentials,
new SimpleChannelProvider(),
emptyClient);

Assert.False(MicrosoftAppCredentials.IsTrustedServiceUrl("https://webchat.botframework.com/"));
}

[Fact]
public async void Emulator_AuthHeader_CorrectAppIdAndServiceUrl_WithGovChannelService_ShouldValidate()
{
Expand Down