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.

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

Caching ClaimsPrincipal during authentication and re-caching FileConfiguration objects in Consul and Disk File configuration repos #1249

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,13 @@ public async Task<Response<FileConfiguration>> Get()
var bytes = queryResult.Response.Value;
var json = Encoding.UTF8.GetString(bytes);
var consulConfig = JsonConvert.DeserializeObject<FileConfiguration>(json);

return new OkResponse<FileConfiguration>(consulConfig);
}

/// <summary>Default TTL in seconds for caching <see cref="FileConfiguration"/> in the <see cref="Set(FileConfiguration)"/> method.</summary>
/// <value>An <see cref="int"/> value, 5 by default.</value>
public static int CacheTtlSeconds { get; set; } = 5;

public async Task<Response> Set(FileConfiguration ocelotConfiguration)
{
var json = JsonConvert.SerializeObject(ocelotConfiguration, Formatting.Indented);
Expand All @@ -70,8 +73,7 @@ public async Task<Response> Set(FileConfiguration ocelotConfiguration)
var result = await _consul.KV.Put(kvPair);
if (result.Response)
{
_cache.AddAndDelete(_configurationKey, ocelotConfiguration, TimeSpan.FromSeconds(3), _configurationKey);

_cache.AddAndDelete(_configurationKey, ocelotConfiguration, TimeSpan.FromSeconds(CacheTtlSeconds), _configurationKey);
return new OkResponse();
}

Expand Down
29 changes: 22 additions & 7 deletions src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs
Original file line number Diff line number Diff line change
@@ -1,21 +1,31 @@
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Caching.Memory;
using Ocelot.Configuration;
using Ocelot.Logging;
using Ocelot.Middleware;
using System.Security.Claims;

namespace Ocelot.Authentication.Middleware
{
public sealed class AuthenticationMiddleware : OcelotMiddleware
{
private readonly RequestDelegate _next;
private readonly IMemoryCache _memoryCache;

public AuthenticationMiddleware(RequestDelegate next, IOcelotLoggerFactory loggerFactory)
public AuthenticationMiddleware(RequestDelegate next,
IOcelotLoggerFactory loggerFactory,
IMemoryCache memoryCache)
: base(loggerFactory.CreateLogger<AuthenticationMiddleware>())
{
_next = next;
_memoryCache = memoryCache;
}

/// <summary>Default TTL in seconds for caching <see cref="ClaimsPrincipal"/> of the current <see cref="AuthenticateResult"/> object.</summary>
/// <value>An <see cref="int"/> value, 300 seconds (5 minutes) by default.</value>
public static int CacheTtlSeconds { get; set; } = 300;

public async Task Invoke(HttpContext httpContext)
{
var request = httpContext.Request;
Expand All @@ -25,23 +35,28 @@ public async Task Invoke(HttpContext httpContext)
// reducing nesting, returning early when no authentication is needed.
if (request.Method.Equals("OPTIONS", StringComparison.OrdinalIgnoreCase) || !downstreamRoute.IsAuthenticated)
{
Logger.LogInformation($"No authentication needed for path '{path}'.");
Logger.LogInformation(() => $"No authentication needed for path '{path}'.");
await _next(httpContext);
return;
}

Logger.LogInformation(() => $"The path '{path}' is an authenticated route! {MiddlewareName} checking if client is authenticated...");
var token = httpContext.Request.Headers.Authorization;
var cacheKey = $"{nameof(AuthenticationMiddleware)}.{nameof(IHeaderDictionary.Authorization)}:{token}";
if (!_memoryCache.TryGetValue(cacheKey, out ClaimsPrincipal principal))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are upgrading with enforced use of the cache❗

{
var auth = await AuthenticateAsync(httpContext, downstreamRoute);
principal = auth.Principal;
_memoryCache.Set(cacheKey, principal, TimeSpan.FromSeconds(CacheTtlSeconds));
}

var result = await AuthenticateAsync(httpContext, downstreamRoute);

if (result.Principal?.Identity == null)
if (principal?.Identity == null)
{
SetUnauthenticatedError(httpContext, path, null);
return;
}

httpContext.User = result.Principal;

httpContext.User = principal;
if (httpContext.User.Identity.IsAuthenticated)
{
Logger.LogInformation(() => $"Client has been authenticated for path '{path}' by '{httpContext.User.Identity.AuthenticationType}' scheme.");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Microsoft.AspNetCore.Hosting;
using Newtonsoft.Json;
using Ocelot.Cache;
using Ocelot.Configuration.ChangeTracking;
using Ocelot.Configuration.File;
using Ocelot.DependencyInjection;
Expand All @@ -12,21 +13,31 @@ public class DiskFileConfigurationRepository : IFileConfigurationRepository
{
private readonly IWebHostEnvironment _hostingEnvironment;
private readonly IOcelotConfigurationChangeTokenSource _changeTokenSource;
private readonly IOcelotCache<FileConfiguration> _cache;

private FileInfo _ocelotFile;
private FileInfo _environmentFile;
private readonly object _lock = new();
public const string CacheKey = nameof(DiskFileConfigurationRepository);

public DiskFileConfigurationRepository(IWebHostEnvironment hostingEnvironment, IOcelotConfigurationChangeTokenSource changeTokenSource)
public DiskFileConfigurationRepository(IWebHostEnvironment hostingEnvironment,
IOcelotConfigurationChangeTokenSource changeTokenSource,
IOcelotCache<FileConfiguration> cache)
{
_hostingEnvironment = hostingEnvironment;
_changeTokenSource = changeTokenSource;
_cache = cache;
Initialize(AppContext.BaseDirectory);
}

public DiskFileConfigurationRepository(IWebHostEnvironment hostingEnvironment, IOcelotConfigurationChangeTokenSource changeTokenSource, string folder)
public DiskFileConfigurationRepository(IWebHostEnvironment hostingEnvironment,
IOcelotConfigurationChangeTokenSource changeTokenSource,
IOcelotCache<FileConfiguration> cache,
string folder)
{
_hostingEnvironment = hostingEnvironment;
_changeTokenSource = changeTokenSource;
_cache = cache;
Initialize(folder);
}

Expand All @@ -42,22 +53,29 @@ private void Initialize(string folder)

public Task<Response<FileConfiguration>> Get()
{
string jsonConfiguration;
var configuration = _cache.Get(CacheKey, region: CacheKey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enforced use of the cache❗

if (configuration != null)
{
return Task.FromResult<Response<FileConfiguration>>(new OkResponse<FileConfiguration>(configuration));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${\color{red}No\ adding\ the\ value\ to\ the\ cache!}$

Current implementation reads the value from the cache, and returns the value if it exists.

When the cache is empty, after reading the file content in lock and deserialization in line 52, we have to add the value to the cache immediately.

So, you forgot to renew the cache by Add method after the line 52:

            lock (_lock)
            {
                jsonConfiguration = System.IO.File.ReadAllText(_environmentFilePath);
            }
            var fileConfiguration = JsonConvert.DeserializeObject<FileConfiguration>(jsonConfiguration);

            // TODO Add this line
            _cache.AddAndDelete(_cacheKey, fileConfiguration, TimeSpan.FromMinutes(5), _cacheKey);
            // OR
            _cache.Add(_cacheKey, fileConfiguration, TimeSpan.FromMinutes(5), _cacheKey);
            // return

string jsonConfiguration;
lock (_lock)
RaynaldM marked this conversation as resolved.
Show resolved Hide resolved
{
jsonConfiguration = FileSys.ReadAllText(_environmentFile.FullName);
}

var fileConfiguration = JsonConvert.DeserializeObject<FileConfiguration>(jsonConfiguration);

return Task.FromResult<Response<FileConfiguration>>(new OkResponse<FileConfiguration>(fileConfiguration));
}

/// <summary>Default TTL in seconds for caching <see cref="FileConfiguration"/> in the <see cref="Set(FileConfiguration)"/> method.</summary>
/// <value>An <see cref="int"/> value, 300 seconds (5 minutes) by default.</value>
public static int CacheTtlSeconds { get; set; } = 300;

public Task<Response> Set(FileConfiguration fileConfiguration)
{
var jsonConfiguration = JsonConvert.SerializeObject(fileConfiguration, Formatting.Indented);

lock (_lock)
{
if (_environmentFile.Exists)
Expand All @@ -76,6 +94,7 @@ public Task<Response> Set(FileConfiguration fileConfiguration)
}

_changeTokenSource.Activate();
_cache.AddAndDelete(CacheKey, fileConfiguration, TimeSpan.FromSeconds(CacheTtlSeconds), region: CacheKey);
return Task.FromResult<Response>(new OkResponse());
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Caching.Memory;
using Ocelot.Authentication.Middleware;
using Ocelot.Configuration;
using Ocelot.Configuration.Builder;
using Ocelot.Logging;
Expand Down Expand Up @@ -49,7 +51,7 @@ public void MiddlewareName_Cstor_ReturnsTypeName()
isNextCalled = true;
return Task.CompletedTask;
};
_middleware = new AuthenticationMiddleware(_next, _factory.Object);
_middleware = new AuthenticationMiddleware(_next, _factory.Object, new MemoryCache(new MemoryCacheOptions()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enforcing the use of new MemoryCache turns the test into an integration test.

var expected = _middleware.GetType().Name;

// Act
Expand Down Expand Up @@ -259,7 +261,7 @@ private async void WhenICallTheMiddleware()
_httpContext.Response.Body = stream;
return Task.CompletedTask;
};
_middleware = new AuthenticationMiddleware(_next, _factory.Object);
_middleware = new AuthenticationMiddleware(_next, _factory.Object, new MemoryCache(new MemoryCacheOptions()));
await _middleware.Invoke(_httpContext);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Caching.Memory;
using Newtonsoft.Json;
using Ocelot.Cache;
using Ocelot.Configuration.ChangeTracking;
using Ocelot.Configuration.File;
using Ocelot.Configuration.Repository;
Expand All @@ -12,29 +14,34 @@ public sealed class DiskFileConfigurationRepositoryTests : FileUnitTest
{
private readonly Mock<IWebHostEnvironment> _hostingEnvironment;
private readonly Mock<IOcelotConfigurationChangeTokenSource> _changeTokenSource;
private readonly Mock<IOcelotCache<FileConfiguration>> _cache;
private IFileConfigurationRepository _repo;
private FileConfiguration _result;

public DiskFileConfigurationRepositoryTests()
{
_hostingEnvironment = new Mock<IWebHostEnvironment>();
_changeTokenSource = new Mock<IOcelotConfigurationChangeTokenSource>(MockBehavior.Strict);
_hostingEnvironment = new();
_changeTokenSource = new();
_cache = new();
_changeTokenSource.Setup(m => m.Activate());
}


// TODO Add integration tests: var aspMemoryCache = new DefaultMemoryCache<FileConfiguration>(new MemoryCache(new MemoryCacheOptions()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add integration tests!

_repo = new DiskFileConfigurationRepository(_hostingEnvironment.Object, _changeTokenSource.Object, /*aspMemoryCache*/_cache.Object);
}

private void Arrange([CallerMemberName] string testName = null)
{
_hostingEnvironment.Setup(he => he.EnvironmentName).Returns(testName);
_repo = new DiskFileConfigurationRepository(_hostingEnvironment.Object, _changeTokenSource.Object, TestID);
_repo = new DiskFileConfigurationRepository(_hostingEnvironment.Object, _changeTokenSource.Object, _cache.Object, TestID);
}

[Fact]
public async Task Should_return_file_configuration()
{
{
Arrange();
var config = FakeFileConfigurationForGet();
GivenTheConfigurationIs(config);

// Act
await WhenIGetTheRoutes();

Expand All @@ -49,7 +56,7 @@ public async Task Should_return_file_configuration_if_environment_name_is_unavai
var config = FakeFileConfigurationForGet();
GivenTheEnvironmentNameIsUnavailable();
GivenTheConfigurationIs(config);

// Act
await WhenIGetTheRoutes();

Expand All @@ -62,7 +69,7 @@ public async Task Should_set_file_configuration()
{
Arrange();
var config = FakeFileConfigurationForSet();

// Act
await WhenISetTheConfiguration(config);

Expand All @@ -81,7 +88,7 @@ public async Task Should_set_file_configuration_if_environment_name_is_unavailab

// Act
await WhenISetTheConfiguration(config);

// Assert
ThenTheConfigurationIsStoredAs(config);
ThenTheConfigurationJsonIsIndented(config);
Expand All @@ -97,15 +104,15 @@ public async Task Should_set_environment_file_configuration_and_ocelot_file_conf

// Act
await WhenISetTheConfiguration(config);

// Assert
ThenTheConfigurationIsStoredAs(config);
ThenTheConfigurationJsonIsIndented(config);
ThenTheOcelotJsonIsStoredAs(ocelotJson, config);
}

private FileInfo GivenTheUserAddedOcelotJson()
{
{
var primaryFile = Path.Combine(TestID, ConfigurationBuilderExtensions.PrimaryConfigFile);
var ocelotJson = new FileInfo(primaryFile);
if (ocelotJson.Exists)
Expand All @@ -120,7 +127,9 @@ private FileInfo GivenTheUserAddedOcelotJson()

private void GivenTheEnvironmentNameIsUnavailable()
{
_hostingEnvironment.Setup(he => he.EnvironmentName).Returns((string)null);
_hostingEnvironment.Setup(he => he.EnvironmentName).Returns(string.Empty);
// TODO Add integration tests: var aspMemoryCache = new DefaultMemoryCache<FileConfiguration>(new MemoryCache(new MemoryCacheOptions()));
//_repo = new DiskFileConfigurationRepository(_hostingEnvironment.Object, _changeTokenSource.Object, aspMemoryCache);
}

private async Task WhenISetTheConfiguration(FileConfiguration fileConfiguration)
Expand Down Expand Up @@ -227,8 +236,8 @@ private static FileConfiguration FakeFileConfigurationForGet()
{
var route = GivenRoute("localhost", "/test/test/{test}");
return GivenConfiguration(route);
}

}
private static FileRoute GivenRoute(string host, string downstream) => new()
{
DownstreamHostAndPorts = new() { new(host, 80) },
Expand Down