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

[Issue] Is there any reason why this can be null? #6

Open
jeffward01 opened this issue Oct 13, 2022 · 1 comment
Open

[Issue] Is there any reason why this can be null? #6

jeffward01 opened this issue Oct 13, 2022 · 1 comment
Assignees
Labels
Investigating Discovery of more details

Comments

@jeffward01
Copy link
Collaborator

Hello,

I was exploring your library and caught an exception during runtime. I had forgot to configure the TimeSpan on the RevokeToken `IserviceCollectionMethod.

RevokeService.cs

    /// <summary>
    ///     Register default InMemory BlackList Store Service using <seealso cref="MemoryCacheBlackList" />
    /// </summary>
    /// <param name="services">The services</param>
    /// <param name="defaultTtl">The default ttl</param>
    /// <returns>The services</returns>
    public static IServiceCollection AddRevokeMemoryCacheStore(this IServiceCollection services, TimeSpan? defaultTtl = null)
    {
        services.TryAddSingleton<IBlackList>(provider => new MemoryCacheBlackList(provider.GetService<IMemoryCache>(), defaultTtl));

        return services;
    }

You know the library better than I do, are there any 'run-time' use-case configurations where defaultTtl will be null and this method will be called?

I understand that null is allowed for this method, however, when Revoke is called while null is configured for defaultTtl an exception is thrown similar to:

"message": "System.ArgumentOutOfRangeException: The added or subtracted value results in an un-representable DateTime. (Parameter 'value')\r\n   at System.DateTime.ThrowDateArithmetic(Int32 param)\r\n   at System.DateTime.AddTicks(Int64 value)\r\n   at System.DateTime.Add(TimeSpan value)\r\n   at Revoke.NET.MemoryBlackList.Revoke(String key)"

I would like to add a null check to ensure that an error is thrown on startup instead of runtime -- However, I am not sure if there is any reason why this will be marked as null during the startup.

Question

Is there any reason why a user will call this method below and be happy with a null value for TimeSpan?

// This or similar... note that the TimeSpan will be null
 services.AddRevokeInMemoryStore().AddJWTBearerTokenRevokeMiddleware();

Propose

  • Option 1: Do nothing, there is a reason why it will be null, ignore it.

  • Option 2: Add a null check that is thrown at runtime if the value is null.

  • Option 3: Remove 'allowed null's for this method

  • Option 4: Add a default value of XX Time in Days or Minutes


You know the library better than I do, what do you suggest?

Thanks

@jeffward01
Copy link
Collaborator Author

Hello,

I see if it is null it can be revoked indefinitely. From the README.md

await store.Revoke(key); // Revoke access indefinetly or with the defaulTtl expiration

I will investigate this further. Thanks

@jeffward01 jeffward01 self-assigned this Oct 13, 2022
@jeffward01 jeffward01 added the Investigating Discovery of more details label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Investigating Discovery of more details
Projects
None yet
Development

No branches or pull requests

1 participant