Skip to content
This repository has been archived by the owner on Dec 13, 2022. It is now read-only.

AbsoluteRefreshTokenLifetime=0 for 1.5.2 #2060

Closed
justmara opened this issue Feb 8, 2018 · 9 comments
Closed

AbsoluteRefreshTokenLifetime=0 for 1.5.2 #2060

justmara opened this issue Feb 8, 2018 · 9 comments
Labels

Comments

@justmara
Copy link

justmara commented Feb 8, 2018

Issue / Steps to reproduce the problem

Currentl documentation says:

AbsoluteRefreshTokenLifetime
Maximum lifetime of a refresh token in seconds. Defaults to 2592000 seconds / 30 days. Zero allows refresh tokens that never expire when used with RefreshTokenExpiration = Sliding.

So if we use this code with 1.5.2 version of library

                client.RefreshTokenExpiration = TokenExpiration.Sliding;
                client.AbsoluteRefreshTokenLifetime = 0;
                client.SlidingRefreshTokenLifetime = 3600;

After first call to /connect/token with grant_type=refresh_token works fine, but resets token LifeTime to zero in storage, so any following request for this token will end up with 'Token expired'.

Relevant parts of the log file

New lifetime exceeds absolute lifetime, capping it to 0

This already was fixed in 2.x branch but many users are (like we do) still bound to aspnecore1.*.
#2049 fixes this

@Iamcerba
Copy link

Iamcerba commented Feb 9, 2018

@justmara, I solved this issue by overriding UpdateRefreshTokenAsync method from DefaultRefreshTokenService. GetLifetimeInSeconds() extension method marked as internal, so just create your own extension.

public class RefreshTokenService : DefaultRefreshTokenService
{
    private readonly ILogger logger;

    /// <summary>
    /// Initializes a new instance of the <see cref="RefreshTokenService"/> class.
    /// </summary>
    /// <param name="refreshTokenStore">The refresh token store</param>
    /// <param name="logger">The logger</param>
    public RefreshTokenService(
        IRefreshTokenStore refreshTokenStore,
        ILogger<DefaultRefreshTokenService> logger
    ) : base(refreshTokenStore, logger)
    {
        this.logger = logger;
    }

    /// <summary>
    /// Updates the refresh token.
    /// </summary>
    /// <param name="handle">The handle.</param>
    /// <param name="refreshToken">The refresh token.</param>
    /// <param name="client">The client.</param>
    /// <returns>
    /// The refresh token handle
    /// </returns>
    public override async Task<string> UpdateRefreshTokenAsync(string handle, RefreshToken refreshToken, Client client)
    {
        logger.LogDebug("Updating refresh token");

        bool needsCreate = false;
        bool needsUpdate = false;

        if (client.RefreshTokenUsage == TokenUsage.OneTimeOnly)
        {
            logger.LogDebug("Token usage is one-time only. Generating new handle");

            // delete old one
            await RefreshTokenStore.RemoveRefreshTokenAsync(handle);

            // create new one
            needsCreate = true;
        }

        if (client.RefreshTokenExpiration == TokenExpiration.Sliding)
        {
            logger.LogDebug("Refresh token expiration is sliding - extending lifetime");

            // if absolute exp > 0, make sure we don't exceed absolute exp
            // if absolute exp = 0, allow indefinite slide
            int currentLifetime = refreshToken.CreationTime.GetLifetimeInSeconds();
            logger.LogDebug("Current lifetime: " + currentLifetime);

            int newLifetime = currentLifetime + client.SlidingRefreshTokenLifetime;
            logger.LogDebug("New lifetime: " + newLifetime);

            // zero absolute refresh token lifetime represents unbounded absolute lifetime
            // if absolute lifetime > 0, cap at absolute lifetime
            if (client.AbsoluteRefreshTokenLifetime > 0 && newLifetime > client.AbsoluteRefreshTokenLifetime)
            {
                newLifetime = client.AbsoluteRefreshTokenLifetime;
                logger.LogDebug("New lifetime exceeds absolute lifetime, capping it to " + newLifetime);
            }

            refreshToken.Lifetime = newLifetime;
            needsUpdate = true;
        }

        if (needsCreate)
        {
            handle = await RefreshTokenStore.StoreRefreshTokenAsync(refreshToken);
            logger.LogDebug("Created refresh token in store");
        }
        else if (needsUpdate)
        {
            await RefreshTokenStore.UpdateRefreshTokenAsync(handle, refreshToken);
            logger.LogDebug("Updated refresh token in store");
        }
        else
        {
            logger.LogDebug("No updates to refresh token done");
        }

        return handle;
    }
}

@brockallen
Copy link
Member

I don't see us adding this back to the 1.x branch. I'd suggest doing what @Iamcerba suggested.

@justmara
Copy link
Author

@brockallen surely I can avoid this misbehavior the way @Iamcerba suggested. BUT 1.5.2 still IS the actual version for aspnet core 1.x and IdentityServer4. And it works not like documentation says. So looks like this is what can surely be called a bug.
So you can add remark to documentation saying "infinite AbsoluteLifeTime can be set only in 2.x package version" or you can merge this bugfix to 1.5.2 so both versions match the documentation.

@leastprivilege
Copy link
Member

leastprivilege commented Feb 11, 2018

@justmara
Copy link
Author

@leastprivilege ohshi... there is version selector! ok then, it looks legit now :) I'll close this issue/pr

@icistrate
Copy link

AMAZING!!! I also fell in the same trap :(

@MCFHTAGENTS
Copy link

I am seeing a similar issue in IdentityServer 2.2 with ASP.NET CORE 2.1.2
https://stackoverflow.com/questions/51684095/how-do-i-stop-identityserver4-refresh-tokens-expiring

@joaopgrassi
Copy link

Just fell as well for this, in an app running ASP.NET Core 1.1 / IdentityServer 1.5.0. Just to confirm: the workaround is to override UpdateRefreshTokenAsync and just register that in my app services.AddTransient<IRefreshTokenService, MyFixedRefreshTokenService>();?

Also: Since others already feel for this (and probably more will), maybe pointing this fact on the latest docs would prevent people from expecting that this works when it doesn't? I know there's a version selector on the docs but it's easy to forget about it.

@lock
Copy link

lock bot commented Jan 11, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

7 participants