Openiddict-core: Implement automatic expired token flushing

[kevinchalet]

(I was pretty sure we already had a work item to track that, but obviously, nope...)

So, we basically have 2 potential options to implement such a feature:

• Remove the expired tokens when receiving a new request.
• Use a dedicated background task/job that would remove the expired tokens from the database at defined intervals. It could potentially be used with something like Hangfire.

I'd love to hear your thoughts about that.

[HolyHorseman]

I had noted that class token has a relation with class application in my question, before new version the FK 'ApplicationId' of class token is null.
In fact, you have another option keeping a token for every application. The relation of application and token is one to one. When process a new request, token can be updated instead of deleted. In other words token is created or updated.
(P.S. The new version is perfect than old. I want to modify the class token, too.)

BTW, about application and scope. They should have a relation. The server must know what the scope of a requesting application is. And application shold have method to add or query scopes.
(P.S. This is new feature that I want to append after migrated.)

[NtFreX]

What about giving the implementer the choise on where he wants to implement it and only provide the functionality to flush?

In my current project I would flush it on a new request but in some time I anyway have to create some cleanup tasks for other entities which would mean I would also switch the flush of the tokens to the cleanup tasks. (I think it will make a difference performance wise on a good used system?)

[kevinchalet]

I took a look at Hangfire and I don't like the API at all (it's static and doesn't play well with DI). I'm not a huge fan of deleting tons of tokens/authorizations during requests (I believe client apps shouldn't pay for deleting tokens issued to other applications).

I'll keep searching, but in the meantime, both the authorization and token managers have been updated to expose all the APIs you need to manually delete orphan authorizations or expired/revoked tokens. Here's a quick and dirty sample of how you could do that in an async void method called from Startup:

public void Configure(IApplicationBuilder app)
{
    // ...

    // Run the authorizations/tokens deletion
    // logic using a fire-and-forget async call.
    FlushEntitiesAsync(app);
}

private async void FlushEntitiesAsync(IApplicationBuilder app)
{
    var lifetime = app.ApplicationServices.GetRequiredService<IApplicationLifetime>();

    while (!lifetime.ApplicationStopping.IsCancellationRequested)
    {
        // Create a new service scope to ensure the database context is correctly disposed when this methods returns.
        using (var scope = app.ApplicationServices.GetRequiredService<IServiceScopeFactory>().CreateScope())
        {
            async Task DeleteAuthorizationsAsync()
            {
                var manager = scope.ServiceProvider.GetRequiredService<OpenIddictAuthorizationManager<OpenIddictAuthorization>>();
                try
                {
                    await manager.PruneAsync(lifetime.ApplicationStopping);
                }

                catch { }
            }

            async Task DeleteTokensAsync()
            {
                var manager = scope.ServiceProvider.GetRequiredService<OpenIddictTokenManager<OpenIddictToken>>();
                try
                {
                    await manager.PruneAsync(lifetime.ApplicationStopping);
                }

                catch { }
            }

            await DeleteAuthorizationsAsync();
            await DeleteTokensAsync();
            await Task.Delay(TimeSpan.FromMinutes(5));
        }
    }
}

Note: for the users of the Orchard OpenID module, we'll have a fully integrated story based on an IBackgroundTask.

[dasMulli]

For ASP.NET Core 2.0, implementing this as an IHostedService would be an option, that way it can just be registered in DI.

[dasMulli]

For 1.0, the integration into the IApplicationLifetime as you showed could be added from within an IStartupFilter that can also be registered form within service configuration time.

[kevinchalet]

@dasMulli thanks for chiming in.

Even if I would personally prefer going with an existing solution offering a fine-grained control on the scheduling part (like Quartz.net or Hangfire), IHostedService is certainly a good option.

It was actually mentioned last month when @Ciantic, @hdesouky and I chatted about this feature on Gitter. @Ciantic was particularly concerned about the fact going with IHostedService for multi-instance apps wouldn't be ideal.

[dasMulli]

@Ciantic was particularly concerned about the fact going with IHostedService for multi-instance apps wouldn't be ideal.

I'm guessing this concern is about added load when all instances are configured identically?

There's always the poor man's approach to distributed scheduling by introducing randomness.. if I have 5 instances, ever instance should wait between cleanups for a random time up to 50 minutes to get an average of 10 minutes between cleans. But I guess if you need to scale up by X instances, you may want to clean up more often so a default random 0-10 wait time might be fine as well..

[IsmailHassani]

In my case i'm using Azure as backend platform which gives me a better solution.
By using Azure Functions you can create a timer based function that calls a webapi endpoint which does the actual pruning.

Step 1.
First i create a controller (name whatever you want)

[AllowAnonymous]
        [HttpGet("~/maintenance/pruneinvalidrecords")]
        public async Task PruneInvalidRecords(CancellationToken cancellationToken = default(CancellationToken))
        {
            using (var scope = Services.GetRequiredService<IServiceScopeFactory>().CreateScope())
            {
                // Note: this task is responsible of automatically removing orphaned tokens/authorizations
                // (i.e tokens that are no longer valid and ad-hoc authorizations that have no valid tokens associated).
                // Since ad-hoc authorizations and their associated tokens are removed as part of the same operation
                // when they no longer have any token attached, it's more efficient to remove the authorizations first.

                // Note: the authorization/token managers MUST be resolved from the scoped provider
                // as they depend on scoped stores that should be disposed as soon as possible.

                try
                {
                    var tokenManager = scope.ServiceProvider.GetRequiredService<OpenIddictTokenManager<OpenIddictToken>>();
                    if (tokenManager != null) await tokenManager.PruneInvalidAsync(cancellationToken);
                }
                catch (Exception) { }

                try
                {
                    var authorizationManager = scope.ServiceProvider.GetRequiredService<OpenIddictAuthorizationManager<OpenIddictAuthorization>>();
                    if (authorizationManager != null) await authorizationManager.PruneInvalidAsync(cancellationToken);
                }
                catch (Exception) { }
            }
        }

Step 2.
Create an Azure Function project with a new function. I set the time at every day at 3AM. But once a week or month should work also.

public static class PruneFunctionRelease
    {
        private static readonly HttpClient httpClient;
        private static readonly string hostname;

        static PruneFunctionRelease()
        {
            hostname = "https://***";
            httpClient = new HttpClient();
        }

        [FunctionName("PruneFunctionRelease")]
        public static async Task Run([TimerTrigger("0 0 3 * * *")]TimerInfo myTimer, TraceWriter log)
        {
            var result = await httpClient.GetAsync($"{hostname}/maintenance/pruneinvalidrecords").ConfigureAwait(false);
            log.Info($"PruneFunctionRelease function executed at: {DateTime.Now}");
        }
    }

Step 3.
Deploy this project to Azure Function Apps
screenshot how it looks in azure
In my case i made 2 functions (test environment and production)

Extra info:
Create your first function
Azure Functions

[kevinchalet]

Out of curiosity, why not merging the API endpoint and the function? It would be better from a security perspective and would have less overhead. E.g:

public static class PruneFunctionRelease
{
    [FunctionName("PruneFunctionRelease")]
    public static async Task Run([TimerTrigger("0 0 3 * * *")] TimerInfo timer, TraceWriter log)
    {
        var services = new ServiceCollection();
        services.AddOpenIddict(options =>
        {
            options.AddEntityFrameworkCoreStores<ApplicationDbContext>();
        });

        services.AddDbContext<ApplicationDbContext>(options =>
        {
            options.UseSqlServer("[connection string]");
            options.UseOpenIddict();
        });

        using (var provider = services.BuildServiceProvider())
        using (var scope = provider.CreateScope())
        {
            try
            {
                await scope.ServiceProvider.GetRequiredService<OpenIddictTokenManager<OpenIddictToken>>()
                    .PruneAsync();
            }

            catch { }

            try
            {
                await scope.ServiceProvider.GetRequiredService<OpenIddictAuthorizationManager<OpenIddictAuthorization>>()
                    .PruneAsync();
            }

            catch { }
        }

        log.Info($"PruneFunctionRelease function executed at: {DateTime.Now}");
    }
}

[IsmailHassani]

Because the function isn't aware of the service like web api.
The function is a simple (serverless) class (linked to Microsoft.NET.Sdk.Functions) without any references to asp.net backend (and service). I thought first i could use this also, but was simply not available. I agree, from security perspective it would be a better solution.
In my example i used an anonymous endpoint, but one could use authentication with the httpclient or restrict access to a certain ip/host.

[kevinchalet]

Azure Functions runs on top of .NET Core 2.0, so you should definitely be able to use OpenIddict's core APIs without having to use ASP.NET Core nor even start a server.

[IsmailHassani]

Ok, that's maybe a good thing to try. ;)
The first error i encountered was regarding the Newtonsoft.Json nuget package which is not compatible with OpenIddict and the version the sdk is using.

[kevinchalet]

Tentatively placing this in the 3.0.0-beta4 milestone to re-evaluate our options.

[kevinchalet]

I took a look at Quartz.NET 3.x and the recent Quartz.Extensions.DependencyInjection/Quartz.Extensions.Hosting packages introduced by @lahma and @andrewlock make it a great candidate for this feature.

However, I have a few concerns:

1. Quartz.NET targets the 3.1 version of the .NET Platform Extensions (logging + hosting) but OpenIddict 3.0 must be usable in any ASP.NET Core 2.1 app running on .NET Core 2.1 and .NET Framework 4.6.1. The good news is that I downloaded Quartz.NET locally and was able to target 2.1 without hitting any issue (only the Quartz.AspNetCore project couldn't target 2.1 as it uses health checks, that were introduced in 2.2, but it's not a package that would be used in OpenIddict itself).

2. It doesn't seem the services.AddQuartz() extensions are idempotent, so if we call them from our integration library to register our custom jobs, it may result in a duplicate service registration when the user calls services.AddQuartz() to configure Quartz.NET (cf. https://github.com/quartznet/quartznet/blob/master/src/Quartz.Extensions.DependencyInjection/ServiceCollectionExtensions.cs#L37)

3. Quartz.NET seems to use static properties/methods for things like logging, which may be problematic in environments like OrchardCore, where the framework must manage multiple root containers (one per tenant) with potentially different settings in the same app domain: https://github.com/quartznet/quartznet/blob/master/src/Quartz.Extensions.DependencyInjection/ServiceCollectionExtensions.cs#L28

4. I didn't find a way to tell Quartz.NET that a given job definition should use a specific IJobFactory, which seems to be a global option only: in our case, we'd likely need a scoped DI container for our jobs, but the factory registered globally may not be scoped if the user opted for a different implementation.

@lahma do you think those are things that could be improved? If so, I'd love to help 😃

[lahma]

Hey thanks for pinging, this looks super interesting and something that should be of course improved. The 3.1 is first stab for these and there's always room for improvement.

1. Quartz.NET targets the 3.1 version of the .NET Platform Extensions (logging + hosting) but OpenIddict 3.0 must be usable in any ASP.NET Core 2.1 app running on .NET Core 2.1 and .NET Framework 4.6.1. The good news is that I downloaded Quartz.NET locally and was able to target 2.1 without hitting any issue (only the Quartz.AspNetCore project couldn't target 2.1 as it uses health checks, that were introduced in 2.2, but it's not a package that would be used in OpenIddict itself).

I too have wondered whether a more open range of packages could/should be used. I see no problem lowering the requirement.

1. It doesn't seem the services.AddQuartz() extensions are idempotent, so if we call it from our integration library to register our custom jobs, it may result in a duplicate service registration when the user calls services.AddQuartz() to configure Quartz.NET (cf. https://github.com/quartznet/quartznet/blob/master/src/Quartz.Extensions.DependencyInjection/ServiceCollectionExtensions.cs#L37)

Didn't consider this, good point. Scheduler factory configuration.should be set in stone after this, but naturally it's OK to add jobs, triggers and probably listeners too.

1. Quartz.NET seems to use static properties/methods for things like logging, which may be problematic in environments like OrchardCore, where the framework must manage multiple root containers (one per tenant) with potentially different settings in the same app domain: https://github.com/quartznet/quartznet/blob/master/src/Quartz.Extensions.DependencyInjection/ServiceCollectionExtensions.cs#L28

Don't have clear idea how to avoid this, currently quartz doesn't get injected loggers for infrastructure, there's only one global scheduler usually inside a process.

1. I didn't find a way to tell Quartz.NET that a given job definition should use a specific IJobFactory, which seems to be a global option only: in our case, we'd likely need a scoped DI container for our jobs, but the factory registered globally may not be scoped if the user opted for a different implementation.

This is also per-scheduler.

I probably need to dig deeper and read about your use case, now just a quick reply via mobile and confirmation that we should try to find a way to improve 👍🏻

[kevinchalet]

@lahma fantastic, thanks for chiming in! 👏

I created a prototype of what a potential OpenIddict/Quartz integration package could look like: https://github.com/openiddict/openiddict-core/pull/1052.

I too have wondered whether a more open range of packages could/should be used. I see no problem lowering the requirement.

Great! Would you like me to send a PR to update Quartz.NET to use the 2.1 .NET Platform Extensions as the minimum version? Quartz.AspNetCore currently references both 3.1 and 2.2 (for health checks), which is not ideal. Since 2.2 is no longer supported and 2.1 doesn't expose the health checks APIs, maybe it should target 3.1?

Didn't consider this, good point. Scheduler factory configuration.should be set in stone after this, but naturally it's OK to add jobs, triggers and probably listeners too.

Ideally, services.AddQuartz() should use the services.TryAdd...() extensions to ensure only a single service type is registered (which would prevent multiple ISchedulerFactory registrations from being added).

One other potential way to improve things Quartz-side would be to use Microsoft.Extensions.Options, which would allow you to have a "fully composable options" story: in this case, you could still amend the scheduler factory options even after calling services.AddQuartz(). Options are used pretty much everywhere in OpenIddict and in Orchard Core so let me know if you're interested in seeing what this looks like in practice.

In my prototype, I opted for manually creating the job/trigger and adding them to the DI container: https://github.com/openiddict/openiddict-core/pull/1052/files#diff-080442f67607bc04bc94254d065a0747R39

Don't have clear idea how to avoid this, currently quartz doesn't get injected loggers for infrastructure, there's only one global scheduler usually inside a process.

I believe I read somewhere that you were considering using the .NET logging stack in 4.0. I guess it would solve that quite naturally if the loggers were resolved from the DI container 😃
Alternatively, maybe you could store the logger instance in some non-static options, so that each container could attach its own logger in a non-static way.

This is also per-scheduler.

Thanks for the info! In the first iteration of https://github.com/openiddict/openiddict-core/pull/1052, I opted for a simple approach: I manually create an IServiceScope in the job itself, which should work with both UseMicrosoftDependencyInjectionJobFactory() and UseMicrosoftDependencyInjectionScopedJobFactory(): https://github.com/openiddict/openiddict-core/pull/1052/files#diff-25871bdc8522f31a38771c0c29d0ad32R69,

[lahma]

After having some sleep and thinking about this, some thoughts.

I created a prototype of what a potential OpenIddict/Quartz integration package could look like: #1052.

Looks nice, I've added some comments.

Great! Would you like me to send a PR to update Quartz.NET to use the 2.1 .NET Platform Extensions as the minimum version? Quartz.AspNetCore currently references both 3.1 and 2.2 (for health checks), which is not ideal. Since 2.2 is no longer supported and 2.1 doesn't expose the health checks APIs, maybe it should target 3.1?

Of course! The minimal which works feels reasonable as I think they ship new versions per .NET Core version without actual bigger changes. Should it be done like Marten does it, e.g. using ranges of allowed values?

Ideally, services.AddQuartz() should use the services.TryAdd...() extensions to ensure only a single service type is registered (which would prevent multiple ISchedulerFactory registrations from being added).

This is something that I'm now circling around a bit.. Because the logic is that scheduler factory is the template how you build a scheduler. But you can validly have multiple schedulers inside the same process (a bit rare though). So it might be that when you add your jobs, you should resolve a named-scheduler (IScheduler with SchedulerName of "my-scheduler") the same goes with scheduler factory so this is a bit tricky as the current Quartz DI implementation now works in and expects the most common case where there's a single scheduler (and factory). Quartz utilizes .NET task pool by default and there's only single long running task (the main scheduler loop) so it shouldn't take too much resources to even run "scheduler per tenant". Database also allows you to host multiple schedulers in same tables (scheduler (name) can have multiple clustered instances (instance id)).

One other potential way to improve things Quartz-side would be to use Microsoft.Extensions.Options, which would allow you to have a "fully composable options" story: in this case, you could still amend the scheduler factory options even after calling services.AddQuartz(). Options are used pretty much everywhere in OpenIddict and in Orchard Core so let me know if you're interested in seeing what this looks like in practice.

As it happens, I just got feedback about options pattern. Someone should definitely show alternative implementation for me to understand it better, this is my first try with this 😄

I believe I read somewhere that you were considering using the .NET logging stack in 4.0. I guess it would solve that quite naturally if the loggers were resolved from the DI container 😃
Alternatively, maybe you could store the logger instance in some non-static options, so that each container could attach its own logger in a non-static way.

This is what I had in mind that could be done as band-aid in 3.x timeframe. I commented on the PR about having property for StdSchedulerFactory, something that would allow having per schedulerfactory loggerfactory - does that make sense? Quartz need to know the implementation and manually inject the logger to places where it's now constructed via static LogProvider log = LogProvider.GetLogger(GetType()) .

Thanks for the info! In the first iteration of #1052, I opted for a simple approach: I manually create an IServiceScope in the job itself, which should work with both UseMicrosoftDependencyInjectionJobFactory() and UseMicrosoftDependencyInjectionScopedJobFactory(): https://github.com/openiddict/openiddict-core/pull/1052/files#diff-25871bdc8522f31a38771c0c29d0ad32R69,

OK I commented about the scoped one, but it seems you have your reasons for your own implementation 🙂

So (after the dependency version decrement) open things still are

• relation of multiple schedulers (and factories) and how to target "your scheduler"
• options pattern
• logging

Feel free to open issues on Quartz side for missing bits, this is really valuable as real-world integration case. I never expected 3.1 to be a perfect release 😄

[kevinchalet]

Should it be done like Marten does it, e.g. using ranges of allowed values?

Personally, I've never been super comfortable with this approach when used "proactively", i.e before you're 100% sure a breaking change introduced in the dependency actually impacts your own package. There are definitely cases where the user may deliberately want to reference a newer version (e.g it's fine to reference the 3.1 or 5.0 version of the .NET Platform Extensions in a .NET Framework or Core console app, for instance).

As it happens, I just got feedback about options pattern. Someone should definitely show alternative implementation for me to understand it better, this is my first try with this 😄

The ASP.NET Core options documentation is a fairly good start but TL;DR:

• The options stack always instantiates your options class for you.
• Options can be amended by registering IConfigureOptions<T>/IConfiguredNamedOptions<T> services that will be invoked in the same order as they are registered.
• You can do post-configuration/validation by registering one or multiple IPostConfigureOptions<T> service, that will be invoked after all the IConfigureOptions<T>/IConfiguredNamedOptions<T>.
• Since these services are resolved from DI, you can use constructor injection. It's massively used in Orchard Core to populate options using database settings. E.g: https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdServerConfiguration.cs#L49

You may be interested in taking a look at how the OpenIddict server ASP.NET Core integration registers itself as an ASP.NET Core authentication handler, as I suspect you may want to leverage the same pattern to register static job details/triggers in Quartz.NET:

• The idempotent options.UseAspNetCore() registers an IConfigureOptions<AuthenticationOptions> using TryAddEnumerable() to ensure it's registered only once:
  https://github.com/openiddict/openiddict-core/blob/22f5632afe35699e678042bea8d40847a17c7b73/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreExtensions.cs#L58-L62

• The OpenIddictServerAspNetCoreConfiguration class implements IConfigureOptions<AuthenticationOptions>
  to register the authentication handler in the ASP.NET Core authentication options: https://github.com/openiddict/openiddict-core/blob/22f5632afe35699e678042bea8d40847a17c7b73/src/OpenIddict.Server.AspNetCore/OpenIddictServerAspNetCoreConfiguration.cs#L23-L43

• ASP.NET Core's AuthenticationSchemeProvider resolves the statically-registered handlers from the options and uses them to populate its dynamic dictionary. New schemes can be dynamically added on the fly using the TryAddScheme method.
  https://github.com/dotnet/aspnetcore/blob/983e7ed635f4d128edce4191b51869bd7be8aced/src/Http/Authentication.Core/src/AuthenticationSchemeProvider.cs#L34-L46

This is what I had in mind that could be done as band-aid in 3.x timeframe. I commented on the PR about having property for StdSchedulerFactory, something that would allow having per schedulerfactory loggerfactory - does that make sense? Quartz need to know the implementation and manually inject the logger to places where it's now constructed via static LogProvider log = LogProvider.GetLogger(GetType()) .

Since static logging is used in many places, maybe it would be simpler to wait for 4.0, where you'll be able to move to a non-static/DI-based approach?

Feel free to open issues on Quartz side for missing bits, this is really valuable as real-world integration case.

Thanks, will definitely do! (likely later today) 🎉

[lahma]

Personally, I've never been super comfortable with this approach when used "proactively", i.e before you're 100% sure a breaking change introduced in the dependency actually impacts your own package. There are definitely cases where the user may deliberately want to reference a newer version (e.g it's fine to reference the 3.1 or 5.0 version of the .NET Platform Extensions in a .NET Framework or Core console app, for instance).

OK so single version it is then 🙂

The ASP.NET Core options documentation is a fairly good start but TL;DR:

Thanks for the reading list, I'll try to dig into this!

Since static logging is used in many places, maybe it would be simpler to wait for 4.0, where you'll be able to move to a non-static/DI-based approach?

Sure if you don't see a stressing need for this functionality 👍

Feel free to open issues on Quartz side for missing bits, this is really valuable as real-world integration case.

Thanks, will definitely do! (likely later today) 🎉

👍