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

Cache the AccessToken in IdentityModelAuthenticationService. #4742

Merged
merged 8 commits into from
Aug 12, 2020
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
<ItemGroup>
<PackageReference Include="IdentityModel" Version="4.3.0" />
<PackageReference Include="Microsoft.Extensions.Http" Version="3.1.5" />
<ProjectReference Include="..\Volo.Abp.Caching\Volo.Abp.Caching.csproj" />
<ProjectReference Include="..\Volo.Abp.MultiTenancy\Volo.Abp.MultiTenancy.csproj" />
<ProjectReference Include="..\Volo.Abp.Threading\Volo.Abp.Threading.csproj" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.Extensions.DependencyInjection;
using Volo.Abp.Caching;
using Volo.Abp.Modularity;
using Volo.Abp.MultiTenancy;
using Volo.Abp.Threading;
Expand All @@ -7,7 +8,8 @@ namespace Volo.Abp.IdentityModel
{
[DependsOn(
typeof(AbpThreadingModule),
typeof(AbpMultiTenancyModule)
typeof(AbpMultiTenancyModule),
typeof(AbpCachingModule)
)]
public class AbpIdentityModelModule : AbpModule
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
using System.Net.Http;
using System.Net.Http.Headers;
using System.Threading.Tasks;
using Microsoft.Extensions.Caching.Distributed;
using Volo.Abp.Caching;
using Volo.Abp.DependencyInjection;
using Volo.Abp.MultiTenancy;
using Volo.Abp.Threading;
Expand All @@ -26,18 +28,21 @@ public class IdentityModelAuthenticationService : IIdentityModelAuthenticationSe
protected IHttpClientFactory HttpClientFactory { get; }
protected ICurrentTenant CurrentTenant { get; }
protected IdentityModelHttpRequestMessageOptions IdentityModelHttpRequestMessageOptions { get; }
protected IDistributedCache<IdentityModelTokenCacheItem> Cache { get; }

public IdentityModelAuthenticationService(
IOptions<AbpIdentityClientOptions> options,
ICancellationTokenProvider cancellationTokenProvider,
IHttpClientFactory httpClientFactory,
ICurrentTenant currentTenant,
IOptions<IdentityModelHttpRequestMessageOptions> identityModelHttpRequestMessageOptions)
IOptions<IdentityModelHttpRequestMessageOptions> identityModelHttpRequestMessageOptions,
IDistributedCache<IdentityModelTokenCacheItem> cache)
{
ClientOptions = options.Value;
CancellationTokenProvider = cancellationTokenProvider;
HttpClientFactory = httpClientFactory;
CurrentTenant = currentTenant;
Cache = cache;
IdentityModelHttpRequestMessageOptions = identityModelHttpRequestMessageOptions.Value;
Logger = NullLogger<IdentityModelAuthenticationService>.Instance;
}
Expand Down Expand Up @@ -76,21 +81,36 @@ public virtual async Task<string> GetAccessTokenAsync(IdentityClientConfiguratio
throw new AbpException($"Could not retrieve the OpenId Connect discovery document! ErrorType: {discoveryResponse.ErrorType}. Error: {discoveryResponse.Error}");
}

var tokenResponse = await GetTokenResponse(discoveryResponse, configuration);

if (tokenResponse.IsError)
var cacheKey = CalculateCacheKey(discoveryResponse, configuration);
Copy link
Member

Choose a reason for hiding this comment

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

Why not including the GetDiscoveryResponse call to the cached part? Should we always call it?

Copy link
Member Author

@maliming maliming Jul 17, 2020

Choose a reason for hiding this comment

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

Sorry I forgot it, I will cache it.
But can we use configuration.Authority + /connect/token directly? Because this url is seem fixed(not sure), of course, developers can also rewrite the method to call the api.

var tokenCacheItem = await Cache.GetAsync(cacheKey);
if (tokenCacheItem == null)
{
if (tokenResponse.ErrorDescription != null)
var tokenResponse = await GetTokenResponse(discoveryResponse, configuration);

if (tokenResponse.IsError)
{
throw new AbpException($"Could not get token from the OpenId Connect server! ErrorType: {tokenResponse.ErrorType}. Error: {tokenResponse.Error}. ErrorDescription: {tokenResponse.ErrorDescription}. HttpStatusCode: {tokenResponse.HttpStatusCode}");
if (tokenResponse.ErrorDescription != null)
{
throw new AbpException($"Could not get token from the OpenId Connect server! ErrorType: {tokenResponse.ErrorType}. " +
$"Error: {tokenResponse.Error}. ErrorDescription: {tokenResponse.ErrorDescription}. HttpStatusCode: {tokenResponse.HttpStatusCode}");
}

var rawError = tokenResponse.Raw;
var withoutInnerException = rawError.Split(new string[] { "<eof/>" }, StringSplitOptions.RemoveEmptyEntries);
throw new AbpException(withoutInnerException[0]);
}

var rawError = tokenResponse.Raw;
var withoutInnerException = rawError.Split(new string[] { "<eof/>" }, StringSplitOptions.RemoveEmptyEntries);
throw new AbpException(withoutInnerException[0]);
await Cache.SetAsync(cacheKey, new IdentityModelTokenCacheItem(tokenResponse.AccessToken),
new DistributedCacheEntryOptions()
{
//Subtract 10 seconds of network request time.
AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(tokenResponse.ExpiresIn - 10)
maliming marked this conversation as resolved.
Show resolved Hide resolved
});

return tokenResponse.AccessToken;
}

return tokenResponse.AccessToken;
return tokenCacheItem.AccessToken;
}

protected virtual void SetAccessToken(HttpClient client, string accessToken)
Expand Down Expand Up @@ -209,5 +229,10 @@ protected virtual void AddHeaders(HttpClient client)
client.DefaultRequestHeaders.Add(TenantResolverConsts.DefaultTenantKey, CurrentTenant.Id.Value.ToString());
}
}

protected virtual string CalculateCacheKey(DiscoveryDocumentResponse discoveryResponse, IdentityClientConfiguration configuration)
{
return IdentityModelTokenCacheItem.CalculateCacheKey(discoveryResponse, configuration);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System;
using System.Linq;
using IdentityModel.Client;
using Volo.Abp.MultiTenancy;

namespace Volo.Abp.IdentityModel
{
[Serializable]
[IgnoreMultiTenancy]
public class IdentityModelTokenCacheItem
{
public string AccessToken { get; set; }

public IdentityModelTokenCacheItem()
{

}

public IdentityModelTokenCacheItem(string accessToken)
{
AccessToken = accessToken;
}

public static string CalculateCacheKey(DiscoveryDocumentResponse discoveryResponse, IdentityClientConfiguration configuration)
{
return discoveryResponse.TokenEndpoint + string.Join(",", configuration.Select(x => x.Key + ":" + x.Value));
Copy link
Member

Choose a reason for hiding this comment

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

Instead of such a long key (which may have problems with cache providers), can we get MD5 (or use a better algorithm) hash of the string. I am just thiking and not sure about it. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the name of the key does not need to have any meaning, md5 is no problem.

}
}
}