Investigation - be more DI friendly #275

Closed
ilmax opened this Issue May 20, 2016 · 10 comments

Comments

4 participants
@ilmax
Contributor

ilmax commented May 20, 2016

This issue came out of a discussion with @PinpointTownes, essentially it would be (IMO) much more user friendly to allow OpenIdConnectServerProvider to be a simple service and move away from the service locator.

To make this possible we should change OpenIdConnectServerOptions to remove the property

public IOpenIdConnectServerProvider Provider { get; set; } = new OpenIdConnectServerProvider();

this should be substituted by something else (e.g. the IOpenIdConnectServerProvider Type that the consumer would use.
Instead of: options.Provider = new MyProvider(); there could be something that options.ProviderType = typeof(MyProvider);

This change allows ASOS to use the IOpenIdConnectServerProvider as a service thus making everyone's life much better :)

Note from @PinpointTownes

One thing to remember is that we need to be able to have 2 ASOS instances in the same pipeline with a different provider for each instance.

It would be inconsistent with the other security middleware in ASP.NET Core. Not a trivial change.

@PinpointTownes PinpointTownes referenced this issue in aspnet/Options May 20, 2016

Closed

Syntax sugar for Func/Delegate/Type #11

@PinpointTownes PinpointTownes added this to the 1.0.0-beta6 milestone May 20, 2016

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes May 20, 2016

Member

Instead of: options.Provider = new MyProvider(); there could be something that options.ProviderType = typeof(MyProvider);

Actually, I guess we should have both, since one must be able to use the delegate approach for simple scenarios:

app.UseOpenIdConnectServer(options => {
    options.Provider = new OpenIdConnectServerProvider {
        // Implement OnValidateAuthorizationRequest to
        // support interactive flows (code/implicit/hybrid).
        OnValidateAuthorizationRequest = async context => { ... },

        // Implement OnValidateTokenRequest to support flows using the token endpoint
        // (code/refresh token/password/client credentials/custom grant).
        OnValidateTokenRequest = async context => { ... }
    };
});
Member

PinpointTownes commented May 20, 2016

Instead of: options.Provider = new MyProvider(); there could be something that options.ProviderType = typeof(MyProvider);

Actually, I guess we should have both, since one must be able to use the delegate approach for simple scenarios:

app.UseOpenIdConnectServer(options => {
    options.Provider = new OpenIdConnectServerProvider {
        // Implement OnValidateAuthorizationRequest to
        // support interactive flows (code/implicit/hybrid).
        OnValidateAuthorizationRequest = async context => { ... },

        // Implement OnValidateTokenRequest to support flows using the token endpoint
        // (code/refresh token/password/client credentials/custom grant).
        OnValidateTokenRequest = async context => { ... }
    };
});
@olivier-spinelli

This comment has been minimized.

Show comment
Hide comment
@olivier-spinelli

olivier-spinelli Aug 3, 2016

Avoiding the locator is important for me too. What about supporting both with an explicit naming like this UseInlineImplementation xor UseProvider:

app.UseOpenIdConnectServer(options => {
    options.UseInlineImplementation( p => {
        // Implement OnValidateAuthorizationRequest to
        // support interactive flows (code/implicit/hybrid).
        p.OnValidateAuthorizationRequest = async context => { ... },

        // Implement OnValidateTokenRequest to support flows using the token endpoint
        // (code/refresh token/password/client credentials/custom grant).
        p.OnValidateTokenRequest = async context => { ... }
     })
});

And:

app.UseOpenIdConnectServer( options => options.UseProvider( typeof(MyProvider) ) );

Of course, an InvalidOperationException should be raised when both are set.
Another option would be to publish 2 different "Use" on the app (app.UseOpenIdConnectServerInline end app.UseOpenIdConnectServerType).

My 2 cents and Thank SO MUCH for this, this is more than great!

Avoiding the locator is important for me too. What about supporting both with an explicit naming like this UseInlineImplementation xor UseProvider:

app.UseOpenIdConnectServer(options => {
    options.UseInlineImplementation( p => {
        // Implement OnValidateAuthorizationRequest to
        // support interactive flows (code/implicit/hybrid).
        p.OnValidateAuthorizationRequest = async context => { ... },

        // Implement OnValidateTokenRequest to support flows using the token endpoint
        // (code/refresh token/password/client credentials/custom grant).
        p.OnValidateTokenRequest = async context => { ... }
     })
});

And:

app.UseOpenIdConnectServer( options => options.UseProvider( typeof(MyProvider) ) );

Of course, an InvalidOperationException should be raised when both are set.
Another option would be to publish 2 different "Use" on the app (app.UseOpenIdConnectServerInline end app.UseOpenIdConnectServerType).

My 2 cents and Thank SO MUCH for this, this is more than great!

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 3, 2016

Member

Salut 😄

Alright, I'll ping the ASP.NET guys and see if/how/when we could adopt a common pattern (since the entire ASP.NET Core security stack is impacted by this design decision). If we can't, we'll likely end up having something specific to ASOS.

@Eilon hey! Any idea on who's the right person to ping for that? 😄

Related tickets: aspnet/Options#11, aspnet/Security#863

Member

PinpointTownes commented Aug 3, 2016

Salut 😄

Alright, I'll ping the ASP.NET guys and see if/how/when we could adopt a common pattern (since the entire ASP.NET Core security stack is impacted by this design decision). If we can't, we'll likely end up having something specific to ASOS.

@Eilon hey! Any idea on who's the right person to ping for that? 😄

Related tickets: aspnet/Options#11, aspnet/Security#863

@olivier-spinelli

This comment has been minimized.

Show comment
Hide comment
@olivier-spinelli

olivier-spinelli Aug 4, 2016

That would be great!
(And, please, forgive me for my second crappy proposal: "to publish 2 different "Use" on the app", this is a terrible idea :).)

That would be great!
(And, please, forgive me for my second crappy proposal: "to publish 2 different "Use" on the app", this is a terrible idea :).)

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 16, 2016

Member

@HaoK looks like you're in charge of aspnet/Security#863. Please ping me when you start working on it, I'd like to coordinate our efforts 😄

Member

PinpointTownes commented Aug 16, 2016

@HaoK looks like you're in charge of aspnet/Security#863. Please ping me when you start working on it, I'd like to coordinate our efforts 😄

@HaoK

This comment has been minimized.

Show comment
Hide comment
@HaoK

HaoK Aug 17, 2016

Sure what are you thinking? For 1.1 we have to be fairly careful to avoid breaking changes, so it will likely be limited to the scope of something like getting the IClaimsTransformer as a service from DI instead of pulling it always from Options. But we can certainly try to do something more elegant...

HaoK commented Aug 17, 2016

Sure what are you thinking? For 1.1 we have to be fairly careful to avoid breaking changes, so it will likely be limited to the scope of something like getting the IClaimsTransformer as a service from DI instead of pulling it always from Options. But we can certainly try to do something more elegant...

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Aug 24, 2016

Member

@HaoK whoooooops, looks like I missed your reply, sorry 😨

My two cents: I believe the entire aspnet/Security stack needs more DI love, not just the claims transformation stack (e.g all the events classes).

@ilmax's approach looks like a good compromise: introduce a new EventsType property that allows one to set the component type that will be resolved from the DI container and fallback to the existing Events property when EventsType is not set.

Member

PinpointTownes commented Aug 24, 2016

@HaoK whoooooops, looks like I missed your reply, sorry 😨

My two cents: I believe the entire aspnet/Security stack needs more DI love, not just the claims transformation stack (e.g all the events classes).

@ilmax's approach looks like a good compromise: introduce a new EventsType property that allows one to set the component type that will be resolved from the DI container and fallback to the existing Events property when EventsType is not set.

@PinpointTownes PinpointTownes referenced this issue in aspnet/Security Oct 24, 2016

Closed

Support IClaimsTransformer from DI #1014

@PinpointTownes PinpointTownes modified the milestones: 1.0.0-rc1, 1.0.0-beta7 Nov 16, 2016

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Nov 16, 2016

Member

Postponed to 1.0.0-rc1 as the DI story for events in ASP.NET Core is still being discussed.

Member

PinpointTownes commented Nov 16, 2016

Postponed to 1.0.0-rc1 as the DI story for events in ASP.NET Core is still being discussed.

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Nov 16, 2016

Member

Note: you can follow the progress here: aspnet/Security#1014.

Member

PinpointTownes commented Nov 16, 2016

Note: you can follow the progress here: aspnet/Security#1014.

@PinpointTownes PinpointTownes modified the milestone: 1.0.0-rc1 Feb 9, 2017

@PinpointTownes PinpointTownes added this to the 2.0.0-preview1 milestone Apr 20, 2017

@PinpointTownes

This comment has been minimized.

Show comment
Hide comment
@PinpointTownes

PinpointTownes Jul 11, 2017

Member

Such support has been added to the ASP.NET Core 2.0 version of ASOS as part of 4a62241 (it won't be implemented in the OWIN/Katana version due to the lack of generic DI support).

The MVC server sample has been updated to demonstrate how to use the new ProviderType property:

Member

PinpointTownes commented Jul 11, 2017

Such support has been added to the ASP.NET Core 2.0 version of ASOS as part of 4a62241 (it won't be implemented in the OWIN/Katana version due to the lack of generic DI support).

The MVC server sample has been updated to demonstrate how to use the new ProviderType property:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment