ASP.NET vNext Identity: Part 9 – Squashing Bugs

I decided to do some basic testing on my ASP.NET vNext Identity code before I move it into my side project (more on that later). I discovered three bugs that I thought were worth mentioning here.

  1. A user can log in without confirming the email address.
  2. When a user registers and then registers again (without confirming), nothing happens.
  3. When a user forgets the password without confirming the email, nothing happens.

Ok – only one of these (the first one) is a bona-fide bug. It’s fixed rather easily by logic within the LoginController.cs file. I’ve altered the top of the POST Index action as follows:

        [HttpPost]
        [AllowAnonymous]
        [ValidateAntiForgeryToken]
        public async Task<IActionResult> Index(LoginViewModel model, string returnUrl = null)
        {
            logger.Trace("POST:Index: Checking ModelState is Valid");
            ViewBag.ReturnUrl = returnUrl;
            if (ModelState.IsValid)
            {
                logger.Trace("POST:Index - does user exist? finding the user");
                var user = await UserManager.FindByNameAsync(model.UserName);
                if (user == null) {
                    logger.Trace("POST:Index - user is not found");
                    ModelState.AddModelError("", "Invalid username or password.");
                    return View(model);
                }
                if (!user.EmailConfirmed) {
                    logger.Trace("POST:Index - user does not have a confirmed email");
                    ModelState.AddModelError("", "Invalid username or password");
                    return View(model);
                }

                logger.Trace("POST:Index - checking password");
                var result = await SignInManager.PasswordSignInAsync(model.UserName, model.Password, model.RememberMe, shouldLockout: false);

The additional logic looks up the user in the SQL database. If the user does not exist or the email is not confirmed, then the whole password sign-in is short-circuited and I just display the form with an “Invalid username or password” error.

For my second bug, one can consider this is the right thing to do. However it’s not very friendly. I don’t have any way of cleaning out un-confirmed accounts yet. What if the user had an email failure or accidentally deleted the email? I decided to add in some logic to the registration process so that if a user tried to register an account that was already registered but not confirmed then the user would be deleted and the new user record would be created. The code goes in RegisterController.cs in the POST Index action:

                logger.Trace("Register: Validating Email Address");
                if (!IsValidEmail(model.Email))
                {
                    logger.Trace(string.Format("Register: Email Address is not valid"));
                    ModelState.AddModelError("", "Invalid email address");
                    return View(model);
                }

                // Check to see if the user is confirmed or not. If the user is not
                // confirmed yet, then assume that the registration failed somehow,
                // delete the user and restart all over again.
                var user = await UserManager.FindByNameAsync(model.Email);
                if (user != null) {
                    logger.Trace("User {0} already exists - checking for confirmation", model.Email);
                    // If not confirmed and not an admin
                    if (!user.EmailConfirmed && !model.Email.Equals("admin")) {
                        logger.Trace("User {0} is not confirmed - resetting registration by deleting user", model.Email);
                        var deleted = await UserManager.DeleteAsync(user);
                        if (!deleted.Succeeded) {
                            logger.Error("Could not delete user {0}", model.Email);
                        }
                    }
                }

                logger.Trace("Register: Creating new ApplicationUser");
                user = new ApplicationUser { UserName = model.Email, Email = model.Email };

The new code is in the middle. As with the login logic update, I first of all look up the user in the SQL database. If the user exists then I check that the user does not have a confirmed email address. I also check to ensure the user is not my admin user. I’ll be adjusting this so that the “admin” is not a literal string at some point. If all the conditions are met, I delete the user via the user manager.

There is somewhat of a security race condition here. Let’s say user-1 is actually registering for the service, but user-2 (a bad user who has previously gained access to the email account of user-1) is also fraudulently registering for the service at the same time. It’s quite possible that the registration of one overwrites the registration of the other with the result that I cannot determine which user is correct. I believe this to be a small concern in the vast majority of web applications. If you have a web application where this is actually a problem, consider another form of registration.

My final bug needs some thinking about. The process is:

  • User registers an account but does not confirm the account
  • User clicks on the forgot password link

What should happen? The user registered an account and then went “what did I type?” My thought is that the user should be redirected to the registration page. The intent is probably to get another code for resetting of that password. Since the account does not fully exist and I have already implemented the code for what happens when registering over the top of an unconfirmed account, I can use this to implement the logic here. The logic is found in the ForgotPasswordController.cs in the POST Index action:

        // POST: /Account/ForgotPassword/Index
        [HttpPost]
        [AllowAnonymous]
        [ValidateAntiForgeryToken]
        public async Task<IActionResult> Index(ForgotPasswordViewModel model)
        {
            if (ModelState.IsValid)
            {
                logger.Trace("POST:Index: Checking for user ID = " + model.Email);
                var user = await UserManager.FindByNameAsync(model.Email);
                // If the user does not exist then say we confirmed, but don't actually do anything.
                if (user == null)
                {
                    logger.Trace("POST:Index: User does not exist - lying to the user");
                    return View("ConfirmationRequired");
                }
                // If the user is not confirmed, then redirect to registration
                if (!user.EmailConfirmed) {
                    logger.Trace("POST:Index: User is not confirmed - redirect to the Registration page");
                    return RedirectToAction("Index", "Register");
                }

                // If we found a user and it's valid, then work out the code and send
                // it via email.
                var code = await UserManager.GeneratePasswordResetTokenAsync(user);
                logger.Trace("POST:Index: Code = " + code);

I used to have a single if statement for when the user did not exist OR the email was not confirmed. Now these two elements are separated.

The short version of this exercise is to think about what users are going to do and what they mean when they do those things. Implement the logic of your workflow accordingly.

In this continuing series on getting Identity right, my code is on my GitHub Repository.

There is still more to be done for Identity. Here is my short list:

  1. Enabling Facebook, Twitter, Microsoft and Google Social Authentication
  2. Automatically cleaning out the dead confirmation records
  3. Utilizing a background task to send email
  4. Utilizing a templating email engine

All of these require features that are not quite available yet in the release branch of ASP.NET and Visual Studio 2015 CTP 6. I will revisit my Identity baseline in the coming months once these are available. Until then, it’s time to do some other stuff, so tomorrow I’ll be introducing my new Web Application.