current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I need to unit test my authentication handler. I don't really want do an assert against the text message returned by the handler. How could this be improved ?

protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
    // Check for API key
    if (!request.Headers.Contains(AuthConfig.ApiKeyHeader))
    {
        return SendResponseText("ApiKey is required");
    }

    // Check for timestamp
    if (request.GetQueryNameValuePairs().Where(k => k.Key.ToLower() == "timestamp").Count() != 1)
    {
        return SendResponseText("Timestamp is required");
    }

    // Validate timestamp
    if (IsValidTimestamp(request) == false)
    {
        return SendResponseText("Invalid timeStamp");
    }

    // Check for signature
    if (request.Headers.Authorization == null || request.Headers.Authorization.Scheme != AuthConfig.AuthenticationScheme)
    {
        return SendResponseText("Signature is required");
    }

    // Validate API Key
    string apikey = request.Headers.GetValues(AuthConfig.ApiKeyHeader).First();
    UserDTO user = _userRepo.GetUserAuthInfo(apikey);
    if (user == null)
    {
        return SendResponseText("Invalid API key");
    }

    // Validate signature
    string signature = _signatureCalculator.GetSignature(user.Secret, request.RequestUri.OriginalString);
    if (request.Headers.Authorization.Parameter != signature)
    {
        return SendResponseText("Invalid signature");
    }

    return base.SendAsync(request, cancellationToken);
}

private Task<HttpResponseMessage> SendResponseText(string text)
{
    Logger.Error(text);

    var response = new HttpResponseMessage(HttpStatusCode.Unauthorized);
    response.ReasonPhrase = text;
    response.Content = new StringContent(text);

    var tcs = new TaskCompletionSource<HttpResponseMessage>();
    tcs.SetResult(response);

    return tcs.Task;
}

So far, I have put the custom reason phrases in a constant

public sealed class AuthError
{
    public const string ApiKeyMissing = "ApiKey is required";
    ...
}

And my tests look like this

[TestMethod]
public void SecurityHandler_ApiKeyMissingFromHeader_ReturnsUnauthorizedHttpStatus()
{
    var handler = new SecurityHandler(new Mock<IClientRepository>().Object, new Mock<ISignatureCalculator>().Object);

    var httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, "http://localhost");

    var client = new HttpClient(handler);
    var result = client.SendAsync(httpRequestMessage).Result;

    Assert.AreEqual(result.StatusCode, HttpStatusCode.Unauthorized);
    Assert.AreEqual(result.ReasonPhrase, SecurityHandler.AuthError.ApiKeyMissing);
}

I'm not too happy with this, and I'm doing 2 asserts which is bad practice.. Any ideas ?

share|improve this question

1 Answer 1

up vote 1 down vote

Basically you need somehow confirm that

  • StatusCode == HttpStatusCode.Unauthorized
  • ReasonPhrase == someDefinedConstant

If you only Assert the first, you can't be sure that the logic is flawless. If you only test the second you can't be sure that StatusCode == HttpStatusCode.Unauthorized.

But again, proper unit tests should fail exactly for one reason.

As others had this problem also, Omer Rauchwerger has created a NUnit addin which can be found here: http://rauchy.net/oapt/

Comments

Comments should be used to describe why something is done. The code itself should describe what is done.
So comments like // Check for API key should be removed and the code following should be extracted to a separate method.

timestamp validation

As the message will be "Timestamp is required" for a failed validation, I guess that you need to check if at least one key is found. There it is better to use Any() over Count().

Style

You should be consistent in your coding style. Here you one time use if (!condition) and another time you use if (condition == false).

Refactoring

Extracted validation methods 

private bool ContainsApiKey(HttpRequestMessage request)
{
    return request.Headers.Contains(AuthConfig.ApiKeyHeader);
}

private bool ContainsTimeStamp(HttpRequestMessage request)
{
    return request.GetQueryNameValuePairs().Any(k => k.Key.ToLower() == "timestamp");
}

private bool ContainsSignature(HttpRequestMessage request)
{
    return (request.Headers.Authorization != null && request.Headers.Authorization.Scheme == AuthConfig.AuthenticationScheme);
}

private UserDTO GetUser(HttpRequestMessage request)
{
    string apikey = request.Headers.GetValues(AuthConfig.ApiKeyHeader).First();
    return  _userRepo.GetUserAuthInfo(apikey);
}

private bool IsValidUser(UserDTO user)
{
    return user != null;
}

private bool IsValidSignature(UserDTO user, HttpRequestMessage request)
{
    string signature = _signatureCalculator.GetSignature(user.Secret, request.RequestUri.OriginalString);
    return  request.Headers.Authorization.Parameter == signature
}

and the cleaned up former method 

protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{

    if (!ContainsApiKey(request))
    {
        return SendResponseText(SecurityHandler.AuthError.ApiKeyMissing);
    }

    if (!ContainsTimeStamp(request))
    {
        return SendResponseText(SecurityHandler.AuthError.TimeStampMissing);
    }

    if (!IsValidTimestamp(request))
    {
        return SendResponseText(SecurityHandler.AuthError.TimeStampInvalid);
    }

    if (!ContainsSignature(request))
    {
        return SendResponseText(SecurityHandler.AuthError.SignatureMissing);
    }

    UserDTO user = GetUser(request);
    if (!IsValidUser(user))
    {
        return SendResponseText(SecurityHandler.AuthError.ApiKeyInvalid);
    }

    if (!IsValidSignature(user,request))
    {
        return SendResponseText(SecurityHandler.AuthError.SignatureInvalid);
    }

    return base.SendAsync(request, cancellationToken);
}
share|improve this answer
    
Thanks for your time, I forgot to mention I'm using MsTest so I can't use that plugin. I don't totally agree on your refactoring as is seems to add a bit of complexity to extract the validation into several one liner methods, but I agree about your other points. –  ThunderDev Dec 9 '14 at 12:49

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.