IdentityServer / IdentityServer3.AccessTokenValidation

OWIN Middleware to validate access tokens from IdentityServer3
Apache License 2.0
91 stars 149 forks source link

Discovery endpoint metadata retrieval: retry behavior #104

Closed vitaliitylyk closed 7 years ago

vitaliitylyk commented 8 years ago

Scenario:

IdentityServer is a separate micro-service, other micro-services use it for authentication via JWT tokens.

  1. Set DelayLoadMetadata = true in middleware options. (Some background: this was done to make Startup of micro-services more reliable and independent from external factors, in this case request to discovery endpoint which might be not available at startup time)
  2. Stop the IdentityServer website.
  3. Make a request to one of the micro-services which use IdentityServer authentication
  4. Start the IdentityServer website.
  5. Make the request again

Expected result: Step 3 fails with

"System.InvalidOperationException: IDX10803: Unable to create to obtain configuration from ... " exception.

Step 5 succeeds, because IdentityServer is up and running again and metadata can be retrieved.

Actual result:

All subsequent requests fail with the same exception. There is no retry behavior for retrieving discovery endpoint metadata.

Cause:

Lazy<T> is used when initializing LocalValidation (Same situation is in ConfigureLocalValidationmethod.):

            if (options.LocalValidationOptions != null)
            {
                _localValidationFunc = new Lazy<AppFunc>(() => 
                {
                    var localBuilder = app.New();
                    localBuilder.UseOAuthBearerAuthentication(options.LocalValidationOptions.Value);
                    localBuilder.Run(ctx => next(ctx.Environment));
                    return localBuilder.Build();

                }, true);
            }

Passing true is the same as using LazyThreadSafetyMode.ExecutionAndPublication. This ensures that only one thread can initialize Lazy, but this also caches exception in case it is thrown during initialization. So all subsequent calls to Lazy.Value will experience the same cached exception thrown every time. In this case the only way to "heal" a micro-service is restart.

I would suggest to use LazyThreadSafetyMode.PublicationOnly mode: multiple threads can invoke the initialization logic but the first thread to complete the initialization successfully sets the value of the Lazy instance. This would make the middleware more fault-tolerant by enabling implicit retry behavior in subsequent requests.

leastprivilege commented 8 years ago

Sounds good. We will look into it.

leastprivilege commented 8 years ago

Have you verified that this works? Could you send a PR?

vitaliitylyk commented 8 years ago

Yes, it works for me. I will send you a PR soon

leastprivilege commented 8 years ago

thanks - against the dev branch please.

vitaliitylyk commented 8 years ago

Okay, PR is here: https://github.com/IdentityServer/IdentityServer3.AccessTokenValidation/pull/106