Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am using idenity server 3 authentication. I have to implement refresh token logic. AccessTokenLifetime is 1 hour, and after that time I want to update access token with refresh token.

I want do this only once, by using filter in mvc. My idea is to check if token has expired, and if yes that get new token.

This is my code, I want to know if this is best practice or you maybe have better idea?

  public class RefreshTokenActionFilter : FilterAttribute, IActionFilter
        {
            void IActionFilter.OnActionExecuted(ActionExecutedContext filterContext)
            {
                var user = filterContext.HttpContext.User as ClaimsPrincipal;
                var request = filterContext.HttpContext.Request;
                RefreshToken(user, request);
            }

            void IActionFilter.OnActionExecuting(ActionExecutingContext filterContext)
            {
            }

            private  void RefreshToken(ClaimsPrincipal User, HttpRequestBase Request)
            {
                var claims = ClaimsPrincipal.Current.Claims;
                var refreshToken = claims.Where(c => c.Type == AuthenticationValues.RefreshTokenKey).Select(c => c.Value).SingleOrDefault();
                long epoch = Convert.ToInt64((claims.Where(c => c.Type == AuthenticationValues.ExpiresAtEpochKey).Select(c => c.Value).SingleOrDefault()));

                // 60 seconds is tolerance
                if (refreshToken != null && epoch - 60 < DateTime.UtcNow.ToEpochTime())
                {
                    var tokenClient = new TokenClient(ApplicationConfiguration.IdentityServerToken,
                                      AuthenticationController.ClientApplicationId,
                                      AuthenticationController.ClientApplicationSecret);

                    var response = (tokenClient.RequestRefreshTokenAsync(refreshToken)).Result;
                    var user = User as ClaimsPrincipal;
                    var identity = user.Identity as ClaimsIdentity;

                    //Remove old tokens:
                    var oldRefreshToken = user.Claims.Single(c => c.Type == AuthenticationValues.RefreshTokenKey);
                    var oldAccessToken = user.Claims.Single(c => c.Type == AuthenticationValues.AccessTokenKey);
                    var oldExpiresAt = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtKey);
                    var oldEpoch = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtEpochKey);
                    identity.RemoveClaim(oldRefreshToken);
                    identity.RemoveClaim(oldAccessToken);
                    identity.RemoveClaim(oldExpiresAt);
                    identity.RemoveClaim(oldEpoch);

                    //Add new tokens:
                    identity.AddClaim(new Claim(AuthenticationValues.AccessTokenKey, response.AccessToken));
                    identity.AddClaim(new Claim(AuthenticationValues.RefreshTokenKey, response.RefreshToken));
                    identity.AddClaim(new Claim(AuthenticationValues.ExpiresAtKey, (DateTime.UtcNow.ToEpochTime() + response.ExpiresIn).ToDateTimeFromEpoch().ToString()));
                    identity.AddClaim(new Claim(AuthenticationValues.ExpiresAtEpochKey, (DateTime.UtcNow.ToEpochTime() + response.ExpiresIn).ToString()));

                    var authenticationManager = Request.GetOwinContext().Authentication;

                    authenticationManager.AuthenticationResponseGrant = new AuthenticationResponseGrant(
                                                                                       new ClaimsPrincipal(identity),
                                                                                       new AuthenticationProperties { IsPersistent = true });
                }
            }
        }
share|improve this question

I can see repetitive code which you can fix easily..

Before

//Remove old tokens:
var oldRefreshToken = user.Claims.Single(c => c.Type == AuthenticationValues.RefreshTokenKey);
var oldAccessToken = user.Claims.Single(c => c.Type == AuthenticationValues.AccessTokenKey);
var oldExpiresAt = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtKey);
var oldEpoch = user.Claims.Single(c => c.Type == AuthenticationValues.ExpiresAtEpochKey);
identity.RemoveClaim(oldRefreshToken);
identity.RemoveClaim(oldAccessToken);
identity.RemoveClaim(oldExpiresAt);
identity.RemoveClaim(oldEpoch);

After

var tokens = user.Claims
    .Where(c =>
        c.Type == AuthenticationValues.RefreshTokenKey ||
        c.Type == AuthenticationValues.AccessTokenKey ||
        c.Type == AuthenticationValues.ExpiresAtKey ||
        c.Type == AuthenticationValues.ExpiresAtEpochKey);

foreach (var token in tokens)
{
    identity.RemoveClaim(token);
}

You have filter claims by its type many times, which is similar code. You can get rid of duplicate code by introducing a method. I am not sure about the return or parameter type but it look somehow like below.

public static string GetClaimsType(string type)
{
    return ClaimsPrincipal.Current.Claims.Where(c => c.Type == type).Select(c => c.Value).SingleOrDefault();
}

Now your code will be much cleaner and shorter..

Before

var claims = ClaimsPrincipal.Current.Claims;
var refreshToken = claims.Where(c => c.Type == AuthenticationValues.RefreshTokenKey).Select(c => c.Value).SingleOrDefault();
long epoch = Convert.ToInt64((claims.Where(c => c.Type == AuthenticationValues.ExpiresAtEpochKey).Select(c => c.Value).SingleOrDefault()));

After

var refreshToken = GetClaimsType(AuthenticationValues.RefreshTokenKey);
long epoch = Convert.ToInt64(GetClaimsType(AuthenticationValues.ExpiresAtEpochKey));
share|improve this answer
    
Answer updated. – SiD Mar 1 at 5:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.