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

Revert the fix introduced in https://github.com/aspnet/Identity/issues/1395 #1483

Closed
kevinchalet opened this issue Oct 26, 2017 · 10 comments

Comments

@kevinchalet
Copy link

kevinchalet commented Oct 26, 2017

When @HaoK fixed #1388, he decided to update the GenerateChangePhoneNumberTokenAsync()/ValidateChangePhoneNumberTokenAsync() methods to directly use the internal Rfc6238AuthenticationService class instead of using the provider configured in the options.

Unfortunately this fix is bad and will break apps using a custom token provider (e.g not based on TOTP) for the "change phone number tokens".

Consider reverting this change and fixing the bug at the root by changing this line to point to the phone provider (by default):

/// <summary>
/// Gets or sets the <see cref="ChangePhoneNumberTokenProvider"/> used to generate tokens used when changing phone numbers.
/// </summary>
/// <value>
/// The <see cref="ChangePhoneNumberTokenProvider"/> used to generate tokens used when changing phone numbers.
/// </value>
public string ChangePhoneNumberTokenProvider { get; set; } = DefaultProvider;

@kevinchalet
Copy link
Author

/cc @blowdart @Eilon @HaoK

@HaoK
Copy link
Member

HaoK commented Oct 26, 2017

Hrm this code was just reverted to what it looked like in 1.1.2

https://github.com/aspnet/Identity/blob/rel/1.1.2/src/Microsoft.AspNetCore.Identity/UserManager.cs#L1550

I guess I must have introduced ChangePhoneNumberTokenProvider in 2.0 to make things consistent at some point, as it doesn't appear to exist in 1.1.2's TokenOptions. https://github.com/aspnet/Identity/blob/rel/1.1.2/src/Microsoft.AspNetCore.Identity/TokenOptions.cs

I'll take a look

@blowdart
Copy link
Member

@Eilon has decided we'll patch this but quirk it.

@kevinchalet
Copy link
Author

A quirk to use the logic added in the "previous" iteration of 2.0.3 or the logic used in 2.0.0?

In both cases, I'm not sure it makes much sense.

@HaoK
Copy link
Member

HaoK commented Oct 26, 2017

Its going to be a quirk that lets you get the old 2.0.0 default value, basically if for whatever reason they want the 2.0 DefaultProvider instead of the DefaultPhoneProvider from token options. Instead of using the quirk they could just set the option too, but this is the pattern that is used for breaking changes generally (add a quirk mode)

@kevinchalet
Copy link
Author

basically if for whatever reason they want the 2.0 DefaultProvider instead of the DefaultPhoneProvider from token options

lol, are there people that are crazy enough to force their users to enter a token protected by Data Protection in a web form?

Adding a quirk doesn't harm anyone, so... :trollface:

@HaoK
Copy link
Member

HaoK commented Oct 26, 2017

I argued against the quirk mode as well, but what can you do :)

@HaoK
Copy link
Member

HaoK commented Oct 26, 2017

Merged into 2.0.3. via aspnet/Universe@60d6045

@HaoK HaoK closed this as completed Oct 26, 2017
@kevinchalet
Copy link
Author

Thanks for fixing that 👍

@Eilon
Copy link
Member

Eilon commented Oct 27, 2017

@PinpointTownes thanks for reporting it! 😄

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

4 participants