search

IdentityServer / IdentityServer3

OpenID Connect Provider and OAuth 2.0 Authorization Server Framework for ASP.NET 4.x/Katana
https://identityserver.github.io/Documentation/
Apache License 2.0
2.01k stars 764 forks source link

Allow for CookieAuthenticationOptions.TicketDataFormat customization #2894

Closed opnarius closed 8 years ago

opnarius commented 8 years ago

It would be helpful for IdentityServer to allow a way to use custom CookieAuthenticationOptions.TicketDataFormat. That way developers can specify their own format, if they choose, otherwise existing functionality is preserved.

I have created a possible extension point in a fork (https://github.com/xpologistics/IdentityServer3/commit/5f40acedda5ce0041db5d961367f816d6b64420a#diff-4427467804ce6635d30c804453812fbf).

The idea is to expose a property on IdentityServerOptions for a delegate that returns instance of ISecureDataFormat<Microsoft.Owin.Security.AuthenticationTicket>. It would take Microsoft.Owin.Security.DataProtection.IDataProtector as input parameter.

The new delegate is passed into UseCookieAuthenticationExtension.ConfigureCookieAuthentication method. If the delegate is null, default TicketDataFormat is used, otherwise the delegate is used to construct a new ISecureDataFormat<Microsoft.Owin.Security.AuthenticationTicket> instance.

Examples: https://github.com/xpologistics/IdentityServer3/blob/master/source/Core/Configuration/IdentityServerOptions.cs#L137-L143 https://github.com/xpologistics/IdentityServer3/blob/master/source/Core/Configuration/AppBuilderExtensions/UseIdentityServerExtension.cs#L87 https://github.com/xpologistics/IdentityServer3/blob/master/source/Core/Configuration/AppBuilderExtensions/ConfigureCookieAuthenticationExtension.cs#L158-L171 https://github.com/xpologistics/IdentityServer3/blob/master/source/Core/Configuration/AppBuilderExtensions/ConfigureCookieAuthenticationExtension.cs#L67-L69

Please review, I would like to get your thoughts on this.

opnarius commented 8 years ago

Because IdentityServer3.dll is ILMerged we can't leak types from Microsoft.Owin.Security assebly as it's being internalized. So to have TicketDataFormat be extendable, one would need to create adapters for all the internilized types, which in my opinion is too muck hassle for a little gain.

What are the chances making Microsoft.Owin.Security as an external dependency, like you have done with Owin?

brockallen commented 8 years ago

What I'm getting at is that the cookie for IdSvr is an internal implementation detail. Why do you want to muck about with the format of it?

brockallen commented 8 years ago

Any update?

opnarius commented 8 years ago

It's really up to you if you want this extension point. I was under some misguided pretenses that took me down the path of overriding default TicketDataFormat. I have found another way of accomplishing the goal.

brockallen commented 8 years ago

Ok, thanks. I don't think we need to expose this level of detail. Thx.

opnarius commented 8 years ago

Thanks Brock. As a side bar, can I ask the reasoning behind ILMerging all the dependencies into one dll? Yes, it makes dependency graph small, but is there other benefits you found?

brockallen commented 8 years ago

It avoids version conflicts in anything your host loads.