3
\$\begingroup\$

There is backend which connects with IdentityServer using WsFederation. The backend is .NET Core project where there is a derived class from AuthenticationHandler.

The issue is when the time of user's token is expired, the user should be signed out. I've implemented this in the following way:

protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
{
    var ticket = await EnsureCookieTicket();
    if (ticket == null)
        return AuthenticateResult.Fail("No ticket.");

    //...

    return AuthenticateResult.Success(new AuthenticationTicket(context.Principal, context.Properties, Options.AuthenticationScheme));
}

private Task<AuthenticationTicket> EnsureCookieTicket()
{
    var cookie = Options.CookieManager.GetRequestCookie(Context, Options.CookieName);
    if (String.IsNullOrEmpty(cookie))
        return null;

    var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());
    if (ticket == null)
        return null;

    var currentUtc = Options.SystemClock.UtcNow;
    var issuedUtc = ticket.Properties.IssuedUtc;
    var expiresUtc = ticket.Properties.ExpiresUtc;

    if (expiresUtc != null && expiresUtc.Value < currentUtc)
    {
        //...

        Context.Items.Add("IsExpired", true);
        return null;
    }
}

protected override async Task<bool> HandleUnauthorizedAsync(ChallengeContext context)
{
    if (Context.Items.ContainsKey("IsExpired")
        && Context.Items["IsExpired"].Equals(true))
    {
        // 1 case
        if (!Request.Path.HasValue ||
            !Request.Path.Value.Equals("/index.html", StringComparison.OrdinalIgnoreCase))
        {
            Context.Response.StatusCode = (int)HttpStatusCode.Forbidden;
            var content = Context.Response;

            using (var responseWriter = new StreamWriter(content.Body, Encoding.UTF8))
            {
                responseWriter.Write("The context has expired and can no longer be used");
            }
        }
        // 2 case
        else
        {
            var signoutContext = new SignOutContext(Options.AuthenticationScheme, context.Properties);
            await SignOutAsync(signoutContext);
        }

        return true;
    }

    // generate return url

    // work with singin, if it is signin message 
    // or work with cleanup signout, if it is cleanup signout context            
    // or await Options.Events.RedirectToLogin(redirectContext);
}

There are two cases this code serves (// 1 case and // 2 case):

  • When the user works with the site, suddenly the time is ended and then the backend returns 403. JavaScript on the client machine handles this error and calls Signout as if the user clicks the button Signout himself.
  • Another case is when the user closes the site tab, the time of the user's token is expired, then the user opens the site again and the signout flow happens.

It works, but I count it as the workaround and not so elegant. What do you think?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

This isn't really a bad workaround. In fact, I wouldn't even consider it a workaround. It is now part of the business logic of the application.

The only reason it feels like a workaround is because of your // 1 case and // 2 case comments. Remove those and this is no longer a workaround.

I see the following method:

protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
{
    var ticket = await EnsureCookieTicket();
    if (ticket == null)
        return AuthenticateResult.Fail("No ticket.");

    //...

    return AuthenticateResult.Success(new AuthenticationTicket(context.Principal, context.Properties, Options.AuthenticationScheme));
}

And can't help but wonder what the //... bit is because I see no reason for ticket to be a strong type. This entire method:

private Task<AuthenticationTicket> EnsureCookieTicket()
{
    var cookie = Options.CookieManager.GetRequestCookie(Context, Options.CookieName);
    if (String.IsNullOrEmpty(cookie))
        return null;

    var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());
    if (ticket == null)
        return null;

    var currentUtc = Options.SystemClock.UtcNow;
    var issuedUtc = ticket.Properties.IssuedUtc;
    var expiresUtc = ticket.Properties.ExpiresUtc;

    if (expiresUtc != null && expiresUtc.Value < currentUtc)
    {
        //...

        Context.Items.Add("IsExpired", true);
        return null;
    }
}

Never returns a non-null value either. So I'm guessing the bits you left out are substantially important to understanding why you chose the form you did.

Provided you don't need AuthenticationTicket in the HandleAuthenticateAsync() method, I would change the return result from Task<AuthenticationTicket> to Task<bool>. Then rename it to IsTicketValid().

if (expiresUtc != null && expiresUtc.Value < currentUtc)

You don't use .HasValue to check there, but you use it on Request.Path.HasValue, we should be consistent and do if (expiresUtc.HasValue && expiresUtc.Value < currentUtc).

I don't like any of these method names, and as a result the method actions themselves because it looks like each one of them has too many responsibilities. You omitted certain parts (so this should have been VTC'd probably) but I'd be willing to bet those parts do more than they should. You should try to find a way to break the flow up into smaller methods that are more maintainable.

\$\endgroup\$
2
  • \$\begingroup\$ "The only reason it feels like a workaround is because of your // 1 case and // 2 case comments. Remove those and this is no longer a workaround." > I asked this question just to review the case 1 and the case 2 =) not to review code as a whole! But thanks for remarks. \$\endgroup\$
    – Barabas
    Jan 23, 2017 at 3:47
  • \$\begingroup\$ @Barabas I meant the only reason it feels like a workaround is because you have those comments. If you remove those two comment lines, it shouldn't feel like a workaround anymore. \$\endgroup\$ Jan 23, 2017 at 4:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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