Switch to services.Initialize<TOptions> #1206
Conversation
|
@HaoK, |
| @@ -48,6 +48,8 @@ protected HttpResponse Response | |||
| /// </summary> | |||
| protected virtual object Events { get; set; } | |||
|
|
|||
| protected virtual string ClaimsIssuer => Options.ClaimsIssuer ?? Scheme.Name; | |||
There was a problem hiding this comment.
So we have to decide whether we require all AddXyz's to chaining to a new AddAuthenticationHandler method which adds an Initializer that just defaults ClaimsIssuer to scheme.Name, or add this new property on the handler which does the defaulting.
I'm leaning towards the property. But we still are going to have chaining for each virtual level of InitializeOptions we have today, so there's going to be an AddRemoteAuthenticationHandler in the next iteration.
| @@ -72,6 +73,8 @@ public string CookieName | |||
|
|
|||
| /// <summary> | |||
| /// If set this will be used by the CookieAuthenticationMiddleware for data protection. | |||
| /// he default data protection service is based on machine key when running on ASP.NET, | |||
There was a problem hiding this comment.
You can just remove this comment, that's an implementation detail of DataProtection.
|
Updated with OIDC/Twitter changes, one nice sideeffect is some of the dependencies for the handlers are no longer needed, namely dataprotection/shared AuthenticationOptions, as those can now just live in the options initializers |
|
|
||
| public static IServiceCollection AddRemoteScheme<TOptions, THandler>(this IServiceCollection services, string authenticationScheme, string displayName, Action<TOptions> configureOptions) | ||
| where TOptions : RemoteAuthenticationOptions, new() | ||
| where THandler : AuthenticationHandler<TOptions> |
|
I really love what's happening here 😄 That said, is there any particular reason we can't get rid of the initialization lock in |
|
Yeah that's the plan @PinpointTownes to get rid of that lock, I just got interrupted and need to finish removing whatevers left. I'll update the PR with the final iteration today |
|
Brilliant! |
| { | ||
| // Makes sure that DataProtectionProvider is always set. | ||
| services.TryAddEnumerable(ServiceDescriptor.Singleton<IInitializeOptions<CookieAuthenticationOptions>, EnsureDataProtection>()); | ||
| services.Initialize<CookieAuthenticationOptions>(authenticationScheme, options => |
There was a problem hiding this comment.
Instead of having the initialization in two places (in a delegate and in a private class), maybe it would be worth consolidating the two and moving everything to a public class? (e.g CookieAuthenticationInitializer)
There was a problem hiding this comment.
Hrm, do we want to allow people to replace/customize this behavior? They already can via adding their own Initializer after, or Configuring it before. I guess it doesn't hurt to leave it public, altho on an unrelated note, all of our handlers are still internal which probably should change now that they actually can be extended
There was a problem hiding this comment.
Hrm, do we want to allow people to replace/customize this behavior?
Actually, that's not the scenario I had in mind. I was just wondering if making the initialization logic wouldn't prevent folks from registering the handler manually (i.e without using the extension), e.g if you don't want the IConfiguration importation thingy. But since the handlers are not public, this is not possible yet, even if you make the initializers public.
HaoK
changed the title
|
Updated with final changes to JwtBearer and removed locking, also combined initializers into a single public type per Auth package. Will merge Options PR + this next week |
|
|
||
| if (options.StateDataFormat == null) | ||
| { | ||
| if (options.DataProtectionProvider == null) |
There was a problem hiding this comment.
@HaoK now that the two initialization routines have been merged, I guess this can be safely removed, no?
There was a problem hiding this comment.
Yep good catch, thanks I'll remove
Goal is to eliminate the virtual AuthenticationHandler.InitializeOptions and the double check locking logic.
Depends on: aspnet/Options#187
Doesn't seem too bad but I had to add a global ensure dataprotection initializer to make things work, definitely is a bit subtle.
I only did Cookies + OAuth and its children for a proof of concept first step. I'll do OIDC next.
cc @Tratcher @ajcvickers