Search code examples
c#asp.net-coreasp.net-identityasp.net-core-mvc

Does Identity Core TwoFactorSignIn contain a bug?


I have been working on an ASP.NET Core application for a couple months. Now near finishing the first beta I realized I hadn't enabled Two-Factor Authentication, and now I think I uncovered a bug in the implementation for Microsoft.AspNetCore.Identity. If we look at how a user is retrieved, it does this:

    /// <summary>
    /// Returns the User ID claim value if present otherwise returns null.
    /// </summary>
    /// <param name="principal">The <see cref="ClaimsPrincipal"/> instance.</param>
    /// <returns>The User ID claim value, or null if the claim is not present.</returns>
    /// <remarks>The User ID claim is identified by <see cref="ClaimTypes.NameIdentifier"/>.</remarks>
    public virtual string GetUserId(ClaimsPrincipal principal)
    {
        if (principal == null)
        {
            throw new ArgumentNullException(nameof(principal));
        }
        return principal.FindFirstValue(Options.ClaimsIdentity.UserIdClaimType);
    }

    /// <summary>
    /// Returns the user corresponding to the IdentityOptions.ClaimsIdentity.UserIdClaimType claim in
    /// the principal or null.
    /// </summary>
    /// <param name="principal">The principal which contains the user id claim.</param>
    /// <returns>The user corresponding to the IdentityOptions.ClaimsIdentity.UserIdClaimType claim in
    /// the principal or null</returns>
    public virtual Task<TUser> GetUserAsync(ClaimsPrincipal principal)
    {
        if (principal == null)
        {
            throw new ArgumentNullException(nameof(principal));
        }
        var id = GetUserId(principal);
        return id == null ? Task.FromResult<TUser>(null) : FindByIdAsync(id);
    }

However, the TwoFactorSignInAsync method in the SignInManager never sets a Claims of type UserIdClaimType, but it sets 4 times the same Name claim, containing the User's Id.
Is this a bug in the implementation of TwoFactorSignInAsync or some configuration is not correct in my configuration of Identity? Which is this:

CookieAuthenticationOptions cookieOptions = new CookieAuthenticationOptions
{
   CookieHttpOnly = true,
   LoginPath = "/User/Login",
   CookieSecure = CookieSecurePolicy.Always,
   LogoutPath = "/User/Logout"
 };

 services.AddIdentity<User, Role>(options =>
 {
     options.Cookies.ApplicationCookie = cookieOptions;
     options.Cookies.ExternalCookie = cookieOptions;
     options.Cookies.TwoFactorRememberMeCookie = cookieOptions;
     options.Cookies.TwoFactorUserIdCookie = cookieOptions;

     options.Password = new PasswordOptions
     {
         RequiredLength = 8,
         RequireLowercase = true,
         RequireUppercase = true,
         RequireNonAlphanumeric = true
     };

     options.SignIn.RequireConfirmedEmail = true;
 })
 .AddUserStore<MyStore>()
 .AddRoleStore<MyStore>()
 .AddDefaultTokenProviders();

For the GitHub issue, please see Does TwoFactorSignIn contain a bug or am I configuring Identity incorrectly? #981


Solution

  • As per @HaoK's comment:

    Two factor sign in if successful, means the NEXT request will have the User set. Authentication for the current request has already happened. None of the SignIn's have no effect on the current request.

    The solution was to remove the GetCurrentUserAsync method after the call to TwoFactorSignInAsync which I incorrectly thought logged in the user immediatly.