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

[PIWEB-21318] Improve access token caching #269

Closed
wants to merge 2 commits into from
Closed

[PIWEB-21318] Improve access token caching #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f99cdb2
[PIWEB-21318] fix: Always store valid access tokens in local cache
daniel-flemming Feb 10, 2025
4d3c888
[PIWEB-21318] feat: Add cancellation support to access token renewal
daniel-flemming Feb 10, 2025
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
19 changes: 10 additions & 9 deletions src/Api.Rest/Common/Utilities/OAuthHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace Zeiss.PiWeb.Api.Rest.Common.Utilities
using System.IO;
using System.Linq;
using System.Security.Claims;
using System.Threading;
using System.Threading.Tasks;
using JetBrains.Annotations;
using Zeiss.PiWeb.Api.Rest.HttpClient.OAuth;
Expand Down Expand Up @@ -225,14 +226,14 @@ private static OAuthTokenCredential TryGetCurrentOAuthToken( string instanceUrl,
/// <cref>https://openid.net/specs/openid-connect-discovery-1_0.html#IssuerDiscovery</cref>
/// </seealso>
/// </summary>
private static async Task<OAuthConfiguration> GetOAuthConfigurationAsync( string instanceUrl )
private static async Task<OAuthConfiguration> GetOAuthConfigurationAsync( string instanceUrl, CancellationToken cancellationToken = default )
{
var oauthServiceRest = new OAuthServiceRestClient( new Uri( instanceUrl ) )
{
UseDefaultWebProxy = true
};

var tokenInformation = await oauthServiceRest.GetOAuthConfiguration().ConfigureAwait( false );
var tokenInformation = await oauthServiceRest.GetOAuthConfiguration( cancellationToken ).ConfigureAwait( false );

if( tokenInformation == null )
throw new InvalidOperationException( "Cannot detect OpenID token information from resource URL." );
Expand All @@ -256,12 +257,14 @@ private static IOidcAuthenticationFlow ChooseSuitableAuthenticationFlow( OAuthCo
/// <param name="refreshToken">Optional refresh token that is used to renew the authentication information.</param>
/// <param name="requestCallbackAsync">Optional callback to request the user to interactively authenticate.</param>
/// <param name="bypassLocalCache">Defines whether locally cached token information are neither used nor updated.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> that can be used to cancel the operation.</param>
/// <returns>A new <see cref="OAuthTokenCredential"/> instance, or <c>null</c>, if no token could be retrieved.</returns>
public static async Task<OAuthTokenCredential> GetAuthenticationInformationForDatabaseUrlAsync(
string databaseUrl,
string refreshToken = null,
Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync = null,
bool bypassLocalCache = false )
bool bypassLocalCache = false,
CancellationToken cancellationToken = default )
{
var instanceUrl = GetInstanceUrl( databaseUrl );

Expand All @@ -272,16 +275,15 @@ public static async Task<OAuthTokenCredential> GetAuthenticationInformationForDa
return cachedToken;
}

var tokenInformation = await GetOAuthConfigurationAsync( instanceUrl ).ConfigureAwait( false );
var tokenInformation = await GetOAuthConfigurationAsync( instanceUrl, cancellationToken ).ConfigureAwait( false );

var authenticationFlow = ChooseSuitableAuthenticationFlow( tokenInformation );
var result = await authenticationFlow.ExecuteAuthenticationFlowAsync( refreshToken, tokenInformation, requestCallbackAsync ).ConfigureAwait( false );
var result = await authenticationFlow.ExecuteAuthenticationFlowAsync( refreshToken, tokenInformation, requestCallbackAsync, cancellationToken ).ConfigureAwait( false );

if( result == null )
return null;

if( !bypassLocalCache )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this line is the actual bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole bypass option exists so that we can store, test and use credential information only from our import plan specific credential storage in AutoImporter. The idea is that the authentication of these automations should be independent from any actions you do in other clients. This will keep working as long as the option still prevents reading the cache. But I'm not sure we want to pollute the cache of other applications with connection configuration in AutoImporter as import automation will most likely use another user. So instead of changing the behavior of the bypass flag, why is the bypass flag used for imports from planner in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and if we decide this change should go through, you need to update the parameter documentation.

Copy link
Contributor

@czthiele czthiele Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also keep in mind that the token cache is using the URL as the key, meaning each "cache skipping" login may overwrite a previously saved and valid token. I also wonder why the import in Planner does this, wouldn't this mean the user has to login everytime? If there is a valid reason, it may be an option to add it more as an opt-in feature, so you have to specify that you want to skip the cache with "bypassLocalCache" and that you want to force-store a new token with something like "forceTokenCacheUpdate", with false as default. So for existing usages there is no change but you can adapt the usage where you need it. This way we also don't need to introduce a breaking change for this logic change. The other changes regarding cancellation are optional parameters and would not require this, a feature increase of the version would be enough.

AccessTokenCache.Store( instanceUrl, result );
AccessTokenCache.Store( instanceUrl, result );

return result;
}
Expand Down Expand Up @@ -317,8 +319,7 @@ public static OAuthTokenCredential GetAuthenticationInformationForDatabaseUrl(
if( result == null )
return null;

if( !bypassLocalCache )
AccessTokenCache.Store( instanceUrl, result );
AccessTokenCache.Store( instanceUrl, result );

return result;
}
Expand Down
15 changes: 10 additions & 5 deletions src/Api.Rest/HttpClient/OAuth/AuthenticationFlows/AuthorizationCodeFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace Zeiss.PiWeb.Api.Rest.HttpClient.OAuth.AuthenticationFlows;

using System;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using IdentityModel.Client;
using Zeiss.PiWeb.Api.Rest.Common.Utilities;
Expand All @@ -32,7 +33,8 @@ private static async Task<OAuthTokenCredential> TryGetOAuthTokenFromAuthorizeRes
CryptoNumbers cryptoNumbers,
AuthorizeResponse response,
OAuthConfiguration configuration,
DiscoveryDocumentResponse discoveryDocument )
DiscoveryDocumentResponse discoveryDocument,
CancellationToken cancellationToken = default )
{
if( response?.Code == null )
return null;
Expand All @@ -44,11 +46,14 @@ private static async Task<OAuthTokenCredential> TryGetOAuthTokenFromAuthorizeRes
var tokenResponse = await tokenClient.RequestAuthorizationCodeTokenAsync(
code: response.Code,
redirectUri: tokenInformation.RedirectUri,
codeVerifier: cryptoNumbers.Verifier ).ConfigureAwait( false );
codeVerifier: cryptoNumbers.Verifier,
cancellationToken: cancellationToken ).ConfigureAwait( false );

if( tokenResponse.IsError )
{
throw new InvalidOperationException(
$"Error during request of access token using authorization code: {tokenResponse.Error}. {tokenResponse.ErrorDescription}." );
}

// decode the IdentityToken claims
var claims = OAuthHelper.DecodeSecurityToken( tokenResponse.IdentityToken ).Claims.ToArray();
Expand Down Expand Up @@ -108,13 +113,13 @@ public OAuthTokenCredential ExecuteAuthenticationFlow( string refreshToken, OAut
}

/// <inheritdoc />
public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string refreshToken, OAuthConfiguration configuration, Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync )
public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string refreshToken, OAuthConfiguration configuration, Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync, CancellationToken cancellationToken )
{
var discoveryInfo = await GetDiscoveryInfoAsync( configuration.UpstreamTokenInformation ).ConfigureAwait( false );
ThrowOnInvalidDiscoveryDocument( discoveryInfo );

var tokenClient = CreateTokenClient( discoveryInfo.TokenEndpoint, configuration.UpstreamTokenInformation.ClientID );
var result = await TryGetOAuthTokenFromRefreshTokenAsync( tokenClient, discoveryInfo.UserInfoEndpoint, refreshToken, configuration ).ConfigureAwait( false );
var result = await TryGetOAuthTokenFromRefreshTokenAsync( tokenClient, discoveryInfo.UserInfoEndpoint, refreshToken, configuration, cancellationToken ).ConfigureAwait( false );
if( result != null )
return result;

Expand All @@ -130,7 +135,7 @@ public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string r

ThrowOnInvalidAuthorizeResponse( response );

result = await TryGetOAuthTokenFromAuthorizeResponseAsync( tokenClient, cryptoNumbers, response, configuration, discoveryInfo ).ConfigureAwait( false );
result = await TryGetOAuthTokenFromAuthorizeResponseAsync( tokenClient, cryptoNumbers, response, configuration, discoveryInfo, cancellationToken ).ConfigureAwait( false );

return result;
}
Expand Down
13 changes: 8 additions & 5 deletions src/Api.Rest/HttpClient/OAuth/AuthenticationFlows/HybridFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Zeiss.PiWeb.Api.Rest.HttpClient.OAuth.AuthenticationFlows;

using System;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using IdentityModel.Client;
using Zeiss.PiWeb.Api.Rest.Common.Utilities;
Expand All @@ -33,7 +34,8 @@ private static async Task<OAuthTokenCredential> TryGetOAuthTokenFromAuthorizeRes
CryptoNumbers cryptoNumbers,
AuthorizeResponse response,
OAuthConfiguration configuration,
DiscoveryDocumentResponse discoveryDocument )
DiscoveryDocumentResponse discoveryDocument,
CancellationToken cancellationToken = default )
{
if( response == null )
return null;
Expand Down Expand Up @@ -69,7 +71,8 @@ private static async Task<OAuthTokenCredential> TryGetOAuthTokenFromAuthorizeRes
var tokenResponse = await tokenClient.RequestAuthorizationCodeTokenAsync(
code: response.Code!,
redirectUri: tokenInformation.RedirectUri,
codeVerifier: cryptoNumbers.Verifier ).ConfigureAwait( false );
codeVerifier: cryptoNumbers.Verifier,
cancellationToken: cancellationToken ).ConfigureAwait( false );

if( tokenResponse.IsError )
throw new InvalidOperationException( $"Error during request of access token using authorization code: {tokenResponse.Error}." );
Expand Down Expand Up @@ -116,13 +119,13 @@ public OAuthTokenCredential ExecuteAuthenticationFlow( string refreshToken, OAut
}

/// <inheritdoc />
public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string refreshToken, OAuthConfiguration configuration, Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync )
public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string refreshToken, OAuthConfiguration configuration, Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync, CancellationToken cancellationToken )
{
var discoveryInfo = await GetDiscoveryInfoAsync( configuration.LocalTokenInformation ).ConfigureAwait( false );
ThrowOnInvalidDiscoveryDocument( discoveryInfo );

var tokenClient = CreateTokenClient( discoveryInfo.TokenEndpoint, configuration.LocalTokenInformation.ClientID );
var result = await TryGetOAuthTokenFromRefreshTokenAsync( tokenClient, discoveryInfo.UserInfoEndpoint, refreshToken, configuration ).ConfigureAwait( false );
var result = await TryGetOAuthTokenFromRefreshTokenAsync( tokenClient, discoveryInfo.UserInfoEndpoint, refreshToken, configuration, cancellationToken ).ConfigureAwait( false );
if( result != null )
return result;

Expand All @@ -138,7 +141,7 @@ public async Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( string r

ThrowOnInvalidAuthorizeResponse( response );

result = await TryGetOAuthTokenFromAuthorizeResponseAsync( tokenClient, cryptoNumbers, response, configuration, discoveryInfo ).ConfigureAwait( false );
result = await TryGetOAuthTokenFromAuthorizeResponseAsync( tokenClient, cryptoNumbers, response, configuration, discoveryInfo, cancellationToken ).ConfigureAwait( false );

return result;
}
Expand Down
5 changes: 4 additions & 1 deletion src/Api.Rest/HttpClient/OAuth/AuthenticationFlows/IOidcAuthenticationFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace Zeiss.PiWeb.Api.Rest.HttpClient.OAuth.AuthenticationFlows;
#region usings

using System;
using System.Threading;
using System.Threading.Tasks;
using JetBrains.Annotations;
using Zeiss.PiWeb.Api.Rest.Common.Utilities;
Expand Down Expand Up @@ -42,9 +43,11 @@ OAuthTokenCredential ExecuteAuthenticationFlow( [CanBeNull] string refreshToken,
/// <param name="refreshToken">Refresh token to acquire a new authentication token.</param>
/// <param name="configuration">OAuth configuration containing the settings for authentication.</param>
/// <param name="requestCallbackAsync">The asynchronous callback to execute for authentication, e.g opening a browser window.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> that can be used to cancel the operation.</param>
Task<OAuthTokenCredential> ExecuteAuthenticationFlowAsync( [CanBeNull] string refreshToken,
OAuthConfiguration configuration,
Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync );
Func<OAuthRequest, Task<OAuthResponse>> requestCallbackAsync,
CancellationToken cancellationToken );

#endregion
}
19 changes: 10 additions & 9 deletions src/Api.Rest/HttpClient/OAuth/AuthenticationFlows/OidcAuthenticationFlowBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Zeiss.PiWeb.Api.Rest.HttpClient.OAuth.AuthenticationFlows;
using System;
using System.Linq;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
using IdentityModel;
using IdentityModel.Client;
Expand Down Expand Up @@ -73,14 +74,14 @@ protected static string ChooseAccessToken( TokenResponse tokenResponse, OAuthCon
/// <param name="tokenInformation">Token information containing the discovery location and other settings.</param>
protected static async Task<DiscoveryDocumentResponse> GetDiscoveryInfoAsync( OAuthTokenInformation tokenInformation )
{
var discoveryCache = new DiscoveryCache( tokenInformation.OpenIdAuthority,
new DiscoveryPolicy
{
AdditionalEndpointBaseAddresses = tokenInformation.AdditionalEndpointBaseAddresses
} );
var discoveryPolicy = new DiscoveryPolicy
{
AdditionalEndpointBaseAddresses = tokenInformation.AdditionalEndpointBaseAddresses
};

var discoveryCache = new DiscoveryCache( tokenInformation.OpenIdAuthority, discoveryPolicy );

var discoveryInfo = await discoveryCache.GetAsync().ConfigureAwait( false );
return discoveryInfo;
return await discoveryCache.GetAsync().ConfigureAwait( false );
}

/// <summary>
Expand Down Expand Up @@ -128,13 +129,13 @@ protected static string CreateOAuthStartUrl( string authorizeEndpoint, string re
/// <param name="refreshToken">Refresh token to acquire a new authentication token.</param>
/// <param name="configuration">OAuth configuration of the PiWeb Server.</param>
/// <returns>A valid <see cref="OAuthTokenCredential"/> or <see langword="null"/> if no token could be retrieved.</returns>
protected static async Task<OAuthTokenCredential> TryGetOAuthTokenFromRefreshTokenAsync( TokenClient tokenClient, string userInfoEndpoint, string refreshToken, OAuthConfiguration configuration )
protected static async Task<OAuthTokenCredential> TryGetOAuthTokenFromRefreshTokenAsync( TokenClient tokenClient, string userInfoEndpoint, string refreshToken, OAuthConfiguration configuration, CancellationToken cancellationToken = default )
{
// when a refresh token is present try to use it to acquire a new access token
if( string.IsNullOrEmpty( refreshToken ) )
return null;

var tokenResponse = await tokenClient.RequestRefreshTokenAsync( refreshToken ).ConfigureAwait( false );
var tokenResponse = await tokenClient.RequestRefreshTokenAsync( refreshToken, cancellationToken: cancellationToken ).ConfigureAwait( false );
if( tokenResponse.IsError )
return null;

Expand Down
Loading