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

Consider requiring callback path #455

Closed
Tratcher opened this issue Sep 16, 2015 · 12 comments
Closed

Consider requiring callback path #455

Tratcher opened this issue Sep 16, 2015 · 12 comments
Assignees
Milestone

Comments

@Tratcher
Copy link
Member

OIDC is the only middleware with an optional callback path, and it's not set by default.

HandleAuthenticateAsync:
if (Options.CallbackPath.HasValue && Options.CallbackPath != (Request.PathBase + Request.Path))

This results in it reading every query string or form body and trying to authenticate.

WsFed needed this this because it could accept logins without first sending a challenge. OIDC doesn't support this because State, Nonce, correlation id, etc. are required.

The only other argument I remember for this design was that developers may not control the value, it may be dicated by the IDP. Even in that case, they should be able to set the path to whatever the IDP requires (usually /).

Consider making callback path required and provide a default value.

@leastprivilege
Copy link
Contributor

Shouldn't the callback not just default to the redirect URI? IOW - the MW should only listen to the redirect URI (then callback path is redundant)

@Eilon Eilon added this to the 1.0.0-beta8 milestone Sep 24, 2015
@Eilon
Copy link
Contributor

Eilon commented Sep 24, 2015

Let's require it and have a default value of /oidc (or similar).

@brentschmaltz
Copy link
Contributor

@leastprivilege are you suggesting Options.RedirectUri? If so, we need to account for DefaultToCurrentUriOnRedirect (if we keep this property #478).

@Tratcher Tratcher assigned HaoK and unassigned Tratcher Oct 12, 2015
@leastprivilege
Copy link
Contributor

Either Callback path is not needed (when you have a single middleware of the same type) - or you need to set it because you have e.g. two OIDC MWs (or WS-Fed) - in this case the Callback is always the same as the redirect URI (or wreply).

That's why I don't see the purpose of this property at all.

Am I missing something?

@Tratcher
Copy link
Member Author

I think it's the redirect uri that we should consider removing. We only had both because it was said IDPs had very strict requirements for it, but we've observed similar requirements for the OAuth providers and they haven't had any trouble using just callback path and generating the redirect uri on demand.

In the worst case if the default generation isn't adequate then we have events you can hook to modify it as needed.

@leastprivilege
Copy link
Contributor

but redirect URI is part of the protocol - callback path a historical leftover because the ws-fed MW "listened" to all incoming requests (to not break backwards compat with WIF FAM).

@HaoK
Copy link
Member

HaoK commented Oct 15, 2015

@Tratcher Do we have consensus on what the plan for this bug is? Callback logic has been pushed into the new RemoteAuthentication base so OIDC is now consistent with the other remote middlewares

@Tratcher
Copy link
Member Author

We still need to rationalize CallbackPath vs RedirectUri. CallbackPath is a mechanical requirement where RedirectUri is a protocol field. I think we could default to deriving CallbackPath from RedirectUri, but that will break if PathBase is not empty (e.g. vdirs, map, etc.).

@HaoK
Copy link
Member

HaoK commented Oct 15, 2015

Do you want to take this issue back since its now an OIDC specific thing it sounds like

@Tratcher
Copy link
Member Author

The more I think about this the less I like how much we differentiate between OIDC and OAuth. The OIDC protocol is doing the same thing as OAuth here so we should use the same middleware mechanics. In this case I think we can just remove RedirectUri and generate it per request like we do for all of the other providers. That makes one less thing for applications to configure, and it's more portable as you move your app between environments.

@HaoK
Copy link
Member

HaoK commented Oct 19, 2015

👍 totally agree that we should try to make them look/feel as similar as possible

@leastprivilege
Copy link
Contributor

I am not opposed to that - if it works like this

  • callback path defaults to some well-known string e.g. "oidc"
  • redirect uri derives from "current base-URL + / + callback path"
  • you can have multiple OIDC middlewares with different callback paths

that would be an improvement over today where redirect URI must be hardcoded AND a callback path must be set if you have multiple MWs. Especially since the callback path must be absolute (ignoring Map paths and IIS hosting paths).

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