3

I'm working on implementing the ForgotPassword functionality ie in the AccountController using ASP.NET Identity as in the standard VS 2015 project template.

The problem I'm trying to solve is that when the password reset email is sent, there is a noticeable delay in the page response. If the password recovery attempt does not find an existing account then no email is sent so there is a faster response. So I think this noticeable delay can be used for account enumeration, that is, a hacker could determine that an account exists based on the response time of the forgot password page.

So I want to eliminate this difference in page response time so that there is no way to detect if an account was found.

In the past I've queued potentially slow tasks like sending an email onto a background thread using code like this:

ThreadPool.QueueUserWorkItem(new WaitCallback(AccountNotification.SendPasswordResetLink), 
notificationInfo);

But ThreadPool.QueueUserWorkItem does not exist in .NET Core, so I'm in need of some alternative.

I suppose one idea is to introduce an artificial delay in the case where no account is found with Thread.Sleep, but I'd rather find a way to send the email without blocking the UI.

UPDATE: To clarify the problem I'm posting the actual code:

[HttpPost]
[AllowAnonymous]
[ValidateAntiForgeryToken]
public async Task<IActionResult> ForgotPassword(ForgotPasswordViewModel model)
{   
    if (ModelState.IsValid)
    {
    var user = await userManager.FindByNameAsync(model.Email);
    if (user == null || !(await userManager.IsEmailConfirmedAsync(user)))
    {
        // Don't reveal that the user does not exist or is not confirmed
        return View("ForgotPasswordConfirmation");
    }

    var code = await userManager.GeneratePasswordResetTokenAsync(user);
    var resetUrl = Url.Action("ResetPassword", "Account", 
        new { userId = user.Id, code = code }, 
        protocol: HttpContext.Request.Scheme);

    //there is a noticeable delay in the UI here because we are awaiting
    await emailSender.SendPasswordResetEmailAsync(
    userManager.Site,
    model.Email,
    "Reset Password",
    resetUrl);


        return View("ForgotPasswordConfirmation");
    }

    // If we got this far, something failed, redisplay form
    return View(model);
}

Is there a good way to handle this using other built in framework functionality?

Joe Audette
  • 35,330
  • 11
  • 106
  • 99
  • Side note: for sending a reminder email, this kind of solution would be acceptable, since if the email never arrived, the user could just request another one. But in general, queueing work to the thread pool on ASP.NET is not recommended for reliability reasons. – Stephen Cleary Feb 03 '16 at 13:57
  • Thanks @Stephen Cleary I was thinking also I better put a try catch in my email code to make sure it doesn't bring down the thread or the process if an error happens on the background thread? – Joe Audette Feb 03 '16 at 14:10
  • Well, error handling is once concern - by default, if you don't await the task, any errors are silently ignored. But I was thinking more about reliability, because any work in ASP.NET that is not tied to a request is not guaranteed to complete. – Stephen Cleary Feb 03 '16 at 14:24
  • 1
    Yes, for things that need to be more reliable would probably want to use something like [hangfire](http://hangfire.io/) or azure web jobs, but hangfire doesn't currently support .NET Core so will have to wait until they do. btw thanks for the great blog posts @stephen-cleary, your articles are some of the best info I find researching this stuff – Joe Audette Feb 03 '16 at 14:36
  • ThreadPool.QueueUserWorkItem appears to exist all the way back to Core 1.0 https://learn.microsoft.com/en-us/dotnet/api/system.threading.threadpool.queueuserworkitem?view=netcore-1.0 – xr280xr Mar 16 '22 at 21:12

1 Answers1

3

Just don't await the task. That's then mostly-equivalent to running all of that code on the thread-pool to start with, assuming it doesn't internally await anything without calling ConfigureAwait(false). (You'll want to check that, if it's your code.)

You might want to add the task to some set of tasks which should be awaited before the server shuts down, assuming there's some appropriate notion of "requested shutdown" in ASP.NET. That's worth looking into, and would stop the notification from being lost due to unfortunate timing of the server being shut down immediately after sending the response but before sending the notification. It wouldn't help in the case where there are problems in sending the notification though, e.g. your mail server is down. At that point, the user has been told that the email is on its way, before you can really guarantee that... just something to think about.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Thanks! Is there a way to do that if the method that sends the email is awaitable, ie AccountNotication.SendPasswordResetLink is an async Task, but I don't want to block the UI waiting on it – Joe Audette Feb 03 '16 at 11:55
  • @JoeAudette: If it's already an async method then it *should* be fast to call; any delay should be asynchronous. If that's not the case, you should revisit the implementation :) – Jon Skeet Feb 03 '16 at 11:56
  • I've updated my question to show the actual current code and I put a comment where it is delayed because of waiting for the email to send – Joe Audette Feb 03 '16 at 12:02
  • @JoeAudette: Ah, I see. Well you could just not await it... that's usually a bad idea, in that you can return the response and then the email could fail to be sent, or the machine could blow up, but that's a different matter... – Jon Skeet Feb 03 '16 at 12:03
  • That concern also existed with ThreadPool.QueueWorkerItem too I think so maybe I should not await and it should be ok unless an error happens – Joe Audette Feb 03 '16 at 12:06
  • not awaiting does solve the UI delay, is it really any worse of an idea than ThreadPool.queueWorkerItem was? and if so what else can I do? If it is ok to just not await please update your answer and I'll mark it as accepted – Joe Audette Feb 03 '16 at 12:13
  • @JoeAudette: No, it's no worse than that. Will update. – Jon Skeet Feb 03 '16 at 12:14
  • Thanks! @JonSkeet your knowledge is legendary! – Joe Audette Feb 03 '16 at 12:23
  • Since it is all my own code would I be better off just making it synchronous and using Task.Run as you originally suggested? That seems like it might be cleaner than making than making the async methods it does call internally use ConfigureAwait(false) – Joe Audette Feb 03 '16 at 12:31
  • nevermind, I see ConfigureAwait(false) is not a problem now that I read up on it – Joe Audette Feb 03 '16 at 12:36