AutomaticTokenManagement doesnt handle refresh token expiration or removal

[LindaLawton]

Question / Steps to reproduce the problem

With AutomaticTokenManagement sample when testing with a refresh token that has been expired or removed. The application just throws an exception rather than rerouting the user back to the login screen.

Minimal working example

AutomaticTokenManagement

Error location

AutomaticTokenManagementCookieEvents.cs#L73

I would expect this to be able to handle this and reroute the user back to the login screen to request a new refresh token. However i suspect this will require logging the user out of the identity server and removing any cookies stored on their machine.

The main issue is if the refresh token was removed in the database there by no longer being valid the user will crash. I also suspect that we do not have sliding refresh tokens enabled which causes refresh tokens to expire and again the application will crash.

At this time I have not been granted permission to enable sliding refresh tokens but this wouldn't help admins revoking refresh tokens anyway.

My first solution.

The only thing i have gotten to work so far is this

[Authorize]
public async Task<IActionResult> CallApi()
{
    try
    {
        var token = await HttpContext.GetTokenAsync("access_token");

        var client = _httpClientFactory.CreateClient();
        client.SetBearerToken(token);

        var response = await client.GetStringAsync(Constants.SampleApi + "identity");
        ViewBag.Json = JArray.Parse(response).ToString();

        return View();
    }
    catch (Exception)
    {
        return new SignOutResult(new[] { "Cookies", "oidc" });
    }
}

While it does work its not ideal IMO as its going to require a try catch on all api calls. That and this is just catching the error from the API call itself and not the fact that the token is still expired after the attempt to refresh it. It would be nice if there was a way to do this from the event handler directly. I would like to update the sample so if anyone has a better idea let me know.

cross post

I have also posted this as a question on Stack overflow Identity server 4 handling Expired or revoked refresh tokens

[leastprivilege]

It's a sample ;)

I probably wouldn't use this approach myself. But rather https://leastprivilege.com/2019/06/30/another-take-on-access-token-management-in-asp-net-core-and-announcing-identitymodel-aspnetcore/

Still you need to deal with this situation somehow.

[LindaLawton]

Unfortunately companies use your samples and expect them to be best practice. If its missing pieces this causes confusion.

The link you posted does not seam to address the Issue of dealing with a refresh token that has expired.

Also are you saying you would note like me to fix this sample when i get it it working?

[brockallen]

Unfortunately companies use your samples and expect them to be best practice. If its missing pieces this causes confusion.

You have a point -- I see all the time companies just grabbing the samples and just using them. But at the same time, we all should know how dangerous that is if the samples are not fully understood. It's like searching for how to fix something and then copying the snippet of code from stackoverflow.

I think @leastprivilege is going to remove that sample. If you'd like to add the error handling, this might be a good place: https://github.com/leastprivilege/AspNetCoreSecuritySamples/tree/aspnetcore21/AutomaticTokenManagement

[LindaLawton]

I hope he doesn't remove it because its linked everywhere. Its currently the best solution for access token refreshing. If he does remove it do you mind my cross posting it to my own blog so that we have a copy to help people.

[dfrunet]

@leastprivilege context.RejectPrincipal(); before return in line 74 should solve the problem. Please don't remove the sample, I've just found it following the link on SO, but it's a really nice addon to MVC! Thank you!

[leastprivilege]

A couple of things about this sample

• I don't like that the logic runs on every request regardless if we call an API or not
• I would rather just call the APIs provided by IdentityModel.AspNetCore from within the event handler than have all the logic there

@dfrunet I guess for this approach - the reject principal is OK since it's all supposed to be "automatic"

@LindaLawton yes - if you fix the sample like mentioned above we can keep it. Send a PR. But don't use the exception handler.

[LindaLawton]

@dfrunet this works perfectly and i dont know why i didnt think of it. Amazing thanks for your help.

@leastprivilege I will add a PR soon I just need to remember the commands for updating my fork. Again ........

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.