Update fix to ChangePhoneNumber

- Added Quirk mode to control default provider - Fixed new regression
aspnet · Oct 26, 2017 · 8c47b90 · 8c47b90
1 parent 98fa107
commit 8c47b90
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 18 deletions.
diff --git a/src/Microsoft.AspNetCore.Identity.Specification.Tests/UserManagerSpecificationTests.cs b/src/Microsoft.AspNetCore.Identity.Specification.Tests/UserManagerSpecificationTests.cs
@@ -1568,12 +1568,48 @@ public async Task ChangePhoneNumberFailsWithWrongToken()
             var stamp = await manager.GetSecurityStampAsync(user);
             IdentityResultAssert.IsFailure(await manager.ChangePhoneNumberAsync(user, "111-111-1111", "bogus"),
                 "Invalid token.");
-            IdentityResultAssert.VerifyLogMessage(manager.Logger, $"VerifyChangePhoneNumberTokenAsync() failed for user { await manager.GetUserIdAsync(user)}.");
+            IdentityResultAssert.VerifyLogMessage(manager.Logger, $"VerifyUserTokenAsync() failed with purpose: ChangePhoneNumber:111-111-1111 for user {await manager.GetUserIdAsync(user)}.");
             Assert.False(await manager.IsPhoneNumberConfirmedAsync(user));
             Assert.Equal("123-456-7890", await manager.GetPhoneNumberAsync(user));
             Assert.Equal(stamp, await manager.GetSecurityStampAsync(user));
         }
 
+        private class YesPhoneNumberProvider : IUserTwoFactorTokenProvider<TUser>
+        {
+            public Task<bool> CanGenerateTwoFactorTokenAsync(UserManager<TUser> manager, TUser user)
+                => Task.FromResult(true);
+
+            public Task<string> GenerateAsync(string purpose, UserManager<TUser> manager, TUser user)
+                => Task.FromResult(purpose);
+
+            public Task<bool> ValidateAsync(string purpose, string token, UserManager<TUser> manager, TUser user)
+                => Task.FromResult(true);
+        }
+
+        /// <summary>
+        /// Test.
+        /// </summary>
+        /// <returns>Task</returns>
+        [Fact]
+        public async Task ChangePhoneNumberWithCustomProvider()
+        {
+            if (ShouldSkipDbTests())
+            {
+                return;
+            }
+            var manager = CreateManager();
+            manager.RegisterTokenProvider("Yes", new YesPhoneNumberProvider());
+            manager.Options.Tokens.ChangePhoneNumberTokenProvider = "Yes";
+            var user = CreateTestUser(phoneNumber: "123-456-7890");
+            IdentityResultAssert.IsSuccess(await manager.CreateAsync(user));
+            Assert.False(await manager.IsPhoneNumberConfirmedAsync(user));
+            var stamp = await manager.GetSecurityStampAsync(user);
+            IdentityResultAssert.IsSuccess(await manager.ChangePhoneNumberAsync(user, "111-111-1111", "whatever"));
+            Assert.True(await manager.IsPhoneNumberConfirmedAsync(user));
+            Assert.Equal("111-111-1111", await manager.GetPhoneNumberAsync(user));
+            Assert.NotEqual(stamp, await manager.GetSecurityStampAsync(user));
+        }
+
         /// <summary>
         /// Test.
         /// </summary>
@@ -1623,7 +1659,8 @@ public async Task CanVerifyPhoneNumber()
             Assert.True(await manager.VerifyChangePhoneNumberTokenAsync(user, token2, num2));
             Assert.False(await manager.VerifyChangePhoneNumberTokenAsync(user, token2, num1));
             Assert.False(await manager.VerifyChangePhoneNumberTokenAsync(user, token1, num2));
-            IdentityResultAssert.VerifyLogMessage(manager.Logger, $"VerifyChangePhoneNumberTokenAsync() failed for user {await manager.GetUserIdAsync(user)}.");
+            IdentityResultAssert.VerifyLogMessage(manager.Logger, $"VerifyUserTokenAsync() failed with purpose: ChangePhoneNumber:{num1} for user {await manager.GetUserIdAsync(user)}.");
+            IdentityResultAssert.VerifyLogMessage(manager.Logger, $"VerifyUserTokenAsync() failed with purpose: ChangePhoneNumber:{num2} for user {await manager.GetUserIdAsync(user)}.");
         }
 
         /// <summary>

diff --git a/src/Microsoft.Extensions.Identity.Core/TokenOptions.cs b/src/Microsoft.Extensions.Identity.Core/TokenOptions.cs
@@ -66,7 +66,7 @@ public class TokenOptions
         /// <value>
         /// The <see cref="ChangePhoneNumberTokenProvider"/> used to generate tokens used when changing phone numbers.
         /// </value>
-        public string ChangePhoneNumberTokenProvider { get; set; } = DefaultProvider;
+        public string ChangePhoneNumberTokenProvider { get; set; } = GetDefaultChangePhoneNumberTokenProvider();
 
         /// <summary>
         /// Gets or sets the <see cref="AuthenticatorTokenProvider"/> used to validate two factor sign ins with an authenticator.
@@ -75,5 +75,9 @@ public class TokenOptions
         /// The <see cref="AuthenticatorTokenProvider"/> used to validate two factor sign ins with an authenticator.
         /// </value>
         public string AuthenticatorTokenProvider { get; set; } = DefaultAuthenticatorProvider;
+
+        private static string GetDefaultChangePhoneNumberTokenProvider()
+            => (AppContext.TryGetSwitch("Microsoft.AspNetCore.Identity.ChangePhoneNumberTokenProvider1483", out var enabled) && enabled)
+               ? DefaultProvider : DefaultPhoneProvider;
     }
 }
diff --git a/src/Microsoft.Extensions.Identity.Core/UserManager.cs b/src/Microsoft.Extensions.Identity.Core/UserManager.cs
@@ -1580,12 +1580,10 @@ public virtual Task<bool> IsPhoneNumberConfirmedAsync(TUser user)
         /// <returns>
         /// The <see cref="Task"/> that represents the asynchronous operation, containing the telephone change number token.
         /// </returns>
-        public virtual async Task<string> GenerateChangePhoneNumberTokenAsync(TUser user, string phoneNumber)
+        public virtual Task<string> GenerateChangePhoneNumberTokenAsync(TUser user, string phoneNumber)
         {
             ThrowIfDisposed();
-            return Rfc6238AuthenticationService.GenerateCode(
-                await CreateSecurityTokenAsync(user), phoneNumber)
-                    .ToString(CultureInfo.InvariantCulture);
+            return GenerateUserTokenAsync(user, Options.Tokens.ChangePhoneNumberTokenProvider, ChangePhoneNumberTokenPurpose + ":" + phoneNumber);
         }
 
         /// <summary>
@@ -1599,21 +1597,16 @@ await CreateSecurityTokenAsync(user), phoneNumber)
         /// The <see cref="Task"/> that represents the asynchronous operation, returning true if the <paramref name="token"/>
         /// is valid, otherwise false.
         /// </returns>
-        public virtual async Task<bool> VerifyChangePhoneNumberTokenAsync(TUser user, string token, string phoneNumber)
+        public virtual Task<bool> VerifyChangePhoneNumberTokenAsync(TUser user, string token, string phoneNumber)
         {
             ThrowIfDisposed();
-
-            var securityToken = await CreateSecurityTokenAsync(user);
-            int code;
-            if (securityToken != null && Int32.TryParse(token, out code))
+            if (user == null)
             {
-                if (Rfc6238AuthenticationService.ValidateCode(securityToken, code, phoneNumber))
-                {
-                    return true;
-                }
+                throw new ArgumentNullException(nameof(user));
             }
-            Logger.LogWarning(8, "VerifyChangePhoneNumberTokenAsync() failed for user {userId}.", await GetUserIdAsync(user));
-            return false;
+
+            // Make sure the token is valid and the stamp matches
+            return VerifyUserTokenAsync(user, Options.Tokens.ChangePhoneNumberTokenProvider, ChangePhoneNumberTokenPurpose+":"+ phoneNumber, token);
         }
 
         /// <summary>

diff --git a/test/Microsoft.AspNetCore.Identity.Test/IdentityOptionsTest.cs b/test/Microsoft.AspNetCore.Identity.Test/IdentityOptionsTest.cs
@@ -14,6 +14,14 @@ namespace Microsoft.AspNetCore.Identity.Test
 {
     public class IdentityOptionsTest
     {
+        [Fact]
+        public void VerifyQuirkChangePhoneNumberTokenProvider1483()
+        {
+            Assert.Equal(TokenOptions.DefaultPhoneProvider, new TokenOptions().ChangePhoneNumberTokenProvider);
+            AppContext.SetSwitch("Microsoft.AspNetCore.Identity.ChangePhoneNumberTokenProvider1483", true);
+            Assert.Equal(TokenOptions.DefaultProvider, new TokenOptions().ChangePhoneNumberTokenProvider);
+        }
+
         [Fact]
         public void VerifyDefaultOptions()
         {