OpenIdConnectOptions API cleanup

[Eilon]

• Rename AuthenticationMethod to RedirectBehavior
• Investigate why DefaultToCurrentUriOnRedirect is needed at all
• Remove HtmlEncoder property and use the service instead. Need to ensure that the service is registered when the OIDC services are registered.

[kevinchalet]

Remove HtmlEncoder property and use the service instead. Need to ensure that the service is registered when the OIDC services are registered.

FYI, this property was added with the POST authorization requests support. I opted for a specific property in the options to allow the developer to replace the global encoder - for instance, to support Russian chars in Razor 😄 - while still being able to use a custom one for the OIDC middleware, where you probably prefer applying a very strict encoding.

---

On a related note, we should strongly consider replacing ResponseType and ResponseMode's default values to use the authorization code flow with GET responses.

Indeed, the authorization code flow offers the most secure approach - since it doesn't imply disclosing the identity/access token to the user agent - and is universally supported.

[Eilon]

Yeah we understood the place where the HtmlEncoder property is used, but felt that having a unique customization for it isn't necessary at this time. That options class has already, what, let's say, 500 different options on it? 😄 We figured removing a very unlikely extensibility scenario would be a good thing (for now). We felt it's far more common to replace it for the entire app.

Re: The OIDC properties, would like to hear from @Tratcher and @brentschmaltz regarding changing the defaults of those two properties.

[kevinchalet]

Yeah we understood the place where the HtmlEncoder property is used, but felt that having a unique customization for it isn't necessary at this time. That options class has already, what, let's say, 500 different options on it? 😄 We figured removing a very unlikely extensibility scenario would be a good thing (for now). We felt it's far more common to replace it for the entire app.

Yeah, it needs a good diet 😄

[HaoK]

Eww, no don't want to do this now :)

[brentschmaltz]

@Eilon DefaultToCurrentUriOnRedirect was added automatically set the OIDC message.RedirectUri to the CurrentUri. This controls where the user-agent sends the post back.

HtmlEncoder was added for the reasons @PinpointTownes mentioned. Before removing, we should consider that IdentityProvider are independent, this is different that say, the CultureInfo for an application where each page will display that same Culture.

[leastprivilege]

So can someone explain the purpose of DefaultToCurrentUriOnRedirect ?

[leastprivilege]

I don't get that - the redirect_uri (as in the oauth or oidc protocol) or wreply (in ws-fed) is a fixed value. This is as fixed as the client_id.

What's the purpose of the new option? Also - do you handle deep links corectly now (like persisting them in a state protected cookie)?

[kevinchalet]

DefaultToCurrentUriOnRedirect's purpose is to control whether the address of the current request will be used to determine the final location of the user agent if AuthenticationProperties.RedirectUri is null. Despite the name, it has nothing to do with the redirect_uri used when communicating with the OP.

That said, the OIDC middleware is the only one having such an option: the other middleware ALWAYS use the current URL as the fallback value when AuthenticationProperties.RedirectUri is null. This property is clearly pointless: #202 (comment)

[leastprivilege]

OK - then I misunderstood that. Maybe because the naming is so confusing ;)

[kevinchalet]

I guess renaming AuthenticationProperties.RedirectUri to AuthenticationProperties.ReturnUrl would help a bit 😄

[HaoK]

@PinpointTownes File a new bug for the RedirectUri => ReturnUrl rename if you feel strongly about that. I'm not opposed :)

[HaoK]

I'm nuking DefaultToCurrentUriOnRedirect as we just do this everywhere else without a flag

[HaoK]

8359038

[DamianEdwards]

This should've had a corresponding announcement issue created as it's a breaking change.