Use AdditionalAuthorizationParameters

[martincostello]

Ensure that overrides of BuildChallengeUrl() that do not call the base implementation add the values from the new AdditionalAuthorizationParameters property.

[kevinchalet]

Move all static challenge parameters into the AdditionalAuthorizationParameters property on the options.

I prefer the old approach: these parameters are actually required for the providers to work correctly so we shouldn't allow users to remove them (which could be accidental, e.g by calling AdditionalAuthorizationParameters.Clear() without realizing the provider implementation now depends on that to send critical parameters)

[martincostello]

we shouldn't allow users to remove them

Didn't consider that, but I did think that it was the more questionable part of the change, hence the review request. I'll undo those parts.

AFAICT, that dictionary can never be null: it's get-only and a default instance is always attached.

I didn't check the definition so assumed it was get-set. Will remove the ?s.

This differs from the OAuthHandler implementation that will throw an exception if you try to add a parameter that the provider already added. Should we change that for consistency?

Depends if you want to remove the ability for the user to be "I know what I'm doing" and override one of our parameters with another value for "{insert reason}". Personally I'd favour the "you can override it if you really want to" behaviour.

[kevinchalet]

I'll undo those parts.

👍🏻

Depends if you want to remove the ability for the user to be "I know what I'm doing" and override one of our parameters with another value for "{insert reason}". Personally I'd favour the "you can override it if you really want to" behaviour.

I have no strong preference/opinion on that (FWIW, I opted for the second option in OpenIddict), but if we don't use the same logic as OAuthHandler, things will be extremely inconsistent: only a few providers here override BuildChallengeUrl, which means all the other ones will use the base implementation provided by OAuthHandler and will exhibit a different logic.

If you think allowing users to override the default parameters is the thing to do, maybe we should try to convince the ASP.NET folks the OAuthHandler implementation should be changed?

/cc @halter73

[halter73]

Depends if you want to remove the ability for the user to be "I know what I'm doing" and override one of our parameters with another value for "{insert reason}". Personally I'd favour the "you can override it if you really want to" behaviour.

I usually like to allow users to say "I know what I'm doing" when working on niche APIs, but a lot of people work with OAuthOptions and OpenIdConnectOptions.

The decision to remove the static challenge parameters from the dictionary so they cannot be accidentally cleared shows that you acknowledge that this API will be used by people who don't know what they're doing and therefore don't want to give them any unnecessary footguns. It seems at least as likely that someone would accidently overwrite a default parameter with a bad value.

If there is any real "{insert reason}" that you can come up with for overwriting a parameter, I'd reconsider. I'm usually more annoyed by being unnecessarily prevented from doing something than I am by footguns.