Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Multiple OIDC handlers will get in each others way. #348

Closed
brentschmaltz opened this issue Jul 9, 2015 · 10 comments
Closed

Multiple OIDC handlers will get in each others way. #348

brentschmaltz opened this issue Jul 9, 2015 · 10 comments

Comments

@brentschmaltz
Copy link
Contributor

Considering the case where a user has two OIDC Middleware each with a different authority and hence different config (keys, issuer, etc).

One will fail, possibly triggering an update to the IDP for new config, throw, add error logs, etc. The other will succeed. Depends on who gets the message first.

We need a way for a MW to know it should handle the signin message.
Solutions could be as simple as using state to add logic: "yes" this is for me. This means crypto for each MW. A "dispatcher / router" or "handlers multiplexing options", etc .. at this layer seems complicated.

@HaoK
Copy link
Member

HaoK commented Jul 9, 2015

Isn't each oidc MW instance having its own authenticationScheme sufficient?

@HaoK
Copy link
Member

HaoK commented Jul 9, 2015

This is handled fine by all the other middlewares today, this should just work...

@Tratcher
Copy link
Member

Tratcher commented Jul 9, 2015

It's fine for challenges, the collision occurs on the (optional) callback path. The other middelware would have the same problem. You either need to set unique callback paths or implement the MessageReceived notification and manually filter them.

@kevinchalet
Copy link
Contributor

@brentschmaltz out of curiosity, why not using CallbackPath (or setting an explicit RedirectUri) to make sure your multiple OIDC middleware don't collide?

@HaoK fine is probably not the most appropriate term 😄
I realized there were actually a few traps when working on the new OpenID 2.0 middleware for ASP.NET 5 yesterday.

Consider this snippet:

app.UseOAuthAuthentication("Scheme1", options => {
    options.Caption = "Caption1";
    options.ClientId = "ClientId1";
    options.ClientSecret = "ClientSecret1";
    options.AuthorizationEndpoint = "AuthorizationEndpoint1";
    options.TokenEndpoint = "TokenEndpoint1";
});

app.UseOAuthAuthentication("Scheme2", options => {
    options.Caption = "Caption2";
    options.ClientId = "ClientId2";
    options.ClientSecret = "ClientSecret2";
    options.AuthorizationEndpoint = "AuthorizationEndpoint2";
    options.TokenEndpoint = "TokenEndpoint2";
});

app.Run(async context => {
    foreach (var scheme in context.Authentication.GetAuthenticationSchemes()) {
        await context.Response.WriteAsync($"{scheme.AuthenticationScheme}: {scheme.Caption}");
    }
});

It will work just fine (GetAuthenticationSchemes will return a description for both Scheme1 and Scheme2), thanks to the first parameter, that sets ConfigureOptions.Name.

But this snippet will miserably fail and only return the options used for the first Google middleware:

app.UseGoogleAuthentication(options => {
    options.AuthenticationScheme = "Scheme1";
    options.Caption = "Caption1";
    options.ClientId = "ClientId1";
    options.ClientSecret = "ClientSecret1";
    options.AuthorizationEndpoint = "AuthorizationEndpoint1";
    options.TokenEndpoint = "TokenEndpoint1";
});

app.UseGoogleAuthentication(options => {
    options.AuthenticationScheme = "Scheme2";
    options.Caption = "Caption2";
    options.ClientId = "ClientId2";
    options.ClientSecret = "ClientSecret2";
    options.AuthorizationEndpoint = "AuthorizationEndpoint2";
    options.TokenEndpoint = "TokenEndpoint2";
});

app.Run(async context => {
    foreach (var scheme in context.Authentication.GetAuthenticationSchemes()) {
        await context.Response.WriteAsync($"{scheme.AuthenticationScheme}: {scheme.Caption}");
    }
});

Of course, this can be fixed using the second parameter (that sets the options instance name), but it's not terribly obvious for a beginner, specially for those who used this pattern with Katana.

@HaoK
Copy link
Member

HaoK commented Jul 9, 2015

Yeah that pain point is well known to me since I ran into (and is what prompted named options in the first place). If you have any suggestions on how to improve that scenario, definitely willing to make it better. Its super hard to discover today, even if you were to look at identity code which is the only one that relies on this, its super subtle and easy to miss...

@HaoK
Copy link
Member

HaoK commented Jul 9, 2015

The issue is actually in the sugar methods, they are implicitly providing "Google"/option names which is why if you don't specify the name, they collide.

@kevinchalet
Copy link
Contributor

@HaoK, actually, I wonder if we shouldn't simply stop retrieving the options from the DI when using the app.Use*Authentication extensions that take an explicit delegate. Why not simply instantiate the options in the extension and manually flow them to the middleware's constructor?

@HaoK
Copy link
Member

HaoK commented Jul 9, 2015

We could certainly stop using IOptions for middlewares, as not all of them do, but there's a consistency issue there where Options are sometimes singletons then, and sometimes instances... Probably should rename them to be Settings and not options just to disambiguate that they aren't like the rest of the options in the framework...

@brentschmaltz
Copy link
Contributor Author

@Tratcher yes the issue is on the way in. In fact un-initiated login updates may occur.
@PinpointTownes it has been my experience that forcing users to have a separate path is a non-starter for some as the IDP may already have registered the redirect_uri.

@Eilon Eilon added this to the 1.0.0-beta8 milestone Jul 30, 2015
@Eilon Eilon modified the milestones: Backlog, 1.0.0-beta8 Jul 30, 2015
@HaoK HaoK modified the milestones: 1.0.0-rc1, Backlog Nov 12, 2015
@HaoK
Copy link
Member

HaoK commented Nov 12, 2015

This is no longer an issue because we require a callback path now, so each middleware can have its own path.

@HaoK HaoK closed this as completed Nov 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants