Add TimeProvider support to token validation

[alexmurari]

Add TimeProvider support to token validation

TimeProvider abstracts time and enables deterministic tests.

Description

Added a new parameter to the TokenValidationParameters class to hold the TimeProvider instance.
The default behavior's still the use of DateTime class to obtain the current time.

Fixes #2572

[alexmurari]

@microsoft-github-policy-service agree

[alexmurari]

TimeProvider is included by default in .NET 8 onwards. For every other target, we must eat the Microsoft.Bcl.TimeProvider nuget.

[alexmurari]

Every test target, except .NET FWK 4.6.1 supports the Microsoft.Extensions.TimeProvider.Testing.

[alexmurari]

Suppress "Microsoft.Bcl.TimeProvider doesn't support .NET Framework 4.6.1, consider upgrading [...]" warnings. (Altough it's compatible with that framework version).

[alexmurari]

Same.

[alexmurari]

Suppresses the "This package may not be fully compatible with your project" warning. The cause is that Microsoft.Extensions.TimeProvider.Testing package is not compatible with .NET FWK 4.6.1.

[pmaytak]

@alexmurari Thanks for the proposal.

After team discussion, there is more design and work needed here. (Our PR process is described in the contributing guide, making sure a design is discussed before time is spent making changes.)

• We're careful with adding new dependencies (MS.Bcl.TimeProvider) as it can create conflicts higher in the stack.
  • So we can conditionally use TimeProvider only in .NET 8.
• We need to make sure adding TimeProvider property to TokenValidationParameters is the right thing to do as it's a commonly used class.

Proposal could be:

• Create an internal time provider abstraction, e.g. IInternalTimeProvider.
• On .NET 8 it will use .NET's TimeProvider, on other platforms - the current DateTime class.
• Update all DateTime related code to use this abstraction, including tests.
• This will allow us more easily to add MS.Bcl.TimeProvider package in the future, if it's the right thing to do.

[alexmurari]

@pmaytak Thanks for the review and the modified proposal.

I agree to those points.

Client code would still need to provide a TimeProvider instance. Since DateTime usage is ubiquitous, we have two possibilities:

a. (Constructor/Method injection) Every class/method that needs a TimeProvider will have new parameters to receive it. (This kinda defeats the IInternalTimeProvider purpose).

- OR -

b. (Property-injection) We create an options class (e.g. TimeProviderOptions) that has an TimeProvider property. It's suitable for property-injection because it has a good local default: TimeProvider.System. It's basically an extension point.

public class TimeProviderOptions
{
    private TimeProvider _timeProvider = TimeProvider.System; // Local default

    public TimeProvider TimeProvider
    {
        get
        {
            return _timeProvider;
        }
        set
        {
            if (value == null) 
               throw new ArgumentNullException("value");

            _timeProvider = value;
        }
    }
}

We just need somewhere to set an instance of this options class, then the IInternalTimeProvider would use it to obtain the TimeProvider instance.

What do you think?

[trejjam]

We used a similar path with TimeProvider in OpenIddict:
https://github.com/openiddict/openiddict-core/pull/2071/files#diff-63fa24a3f983e94df2b11646deac927651bd5a5836f02d3d1de6dbd30f61e5baR1092

One of the constraints was also not to use the Bcl package. So there is #SUPPORTS_TIME_PROVIDER used instead

[trejjam]

This way, the PR becomes simple and minimalistic. I am willing to prepare PR using this approach.

[trejjam]

A workaround can look like this:

private const string TimeProviderName = "TimeProviderName";

/// <summary>
/// Validates the lifetime of a <see cref="SecurityToken"/>.
/// </summary>
internal static bool ValidateLifetime(DateTime? notBefore, DateTime? expires, SecurityToken securityToken, TokenValidationParameters validationParameters)
{
    if (!expires.HasValue && validationParameters.RequireExpirationTime)
    {
        throw LogHelper.LogExceptionMessage(new SecurityTokenNoExpirationException(
            LogHelper.FormatInvariant("IDX10225", LogHelper.MarkAsNonPII(securityToken.GetType().ToString()))
        ));
    }

    if (notBefore.HasValue && expires.HasValue && notBefore.Value > expires.Value)
    {
        throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidLifetimeException(
            LogHelper.FormatInvariant("IDX10224", LogHelper.MarkAsNonPII(notBefore.Value), LogHelper.MarkAsNonPII(expires.Value))
        )
        {
            NotBefore = notBefore,
            Expires = expires,
        });
    }

    var timeProvider = (TimeProvider) validationParameters.InstancePropertyBag[TimeProviderName];

    var utcNow = timeProvider.GetUtcNow().UtcDateTime;
    if (notBefore.HasValue && notBefore.Value > DateTimeUtil.Add(utcNow, validationParameters.ClockSkew))
    {
        throw LogHelper.LogExceptionMessage(new SecurityTokenNotYetValidException(
            LogHelper.FormatInvariant("IDX10222", LogHelper.MarkAsNonPII(notBefore.Value), LogHelper.MarkAsNonPII(utcNow))
        )
        {
            NotBefore = notBefore.Value,
        });
    }

    if (expires.HasValue && expires.Value < DateTimeUtil.Add(utcNow, validationParameters.ClockSkew.Negate()))
    {
        throw LogHelper.LogExceptionMessage(new SecurityTokenExpiredException(
            LogHelper.FormatInvariant("IDX10223", LogHelper.MarkAsNonPII(expires.Value), LogHelper.MarkAsNonPII(utcNow))
        )
        {
            Expires = expires.Value,
        });
    }

    // if it reaches here, that means lifetime of the token is valid
    LogHelper.LogInformation("IDX10239");
    return true;
}

var timeProvider = TimeProvider.System;

var validationParameters = new TokenValidationParameters
{
  LifetimeValidator = ValidateLifetime,
}
validationParameters.InstancePropertyBag.Add(TimeProviderName, timeProvider);

[alexmurari]

@trejjam That's a great approach! The changes to OpenIddict were minimal.

Fine by me a new PR with this approach. I'm just not sure where client code will configure the desired TimeProvider instance.

[trejjam]

Hi my PR is here: #2612

[alexmurari]

Closing in favor of #2612