Simple Google ReCaptcha validation

Question

Linked: Google reCAPTCHA Validator: Iteration II

---

I have also created a simple Google Recaptcha Validation class to handle verification.

I used some code from CodingFusion's post Google New reCaptcha I am not a robot using asp .net, but I have altered it so that it fit my use case, and to make it extendable and reusable.

Can I clean it up further?

public class ReCaptchaValidator
{
    private readonly string _ReCaptchaSecret;
    private readonly string _ReCaptchaSiteKey;
    public List<string> ErrorCodes { get; set; }

    public ReCaptchaValidator(string reCaptchaSecret)
    {
        _ReCaptchaSecret = reCaptchaSecret;
        this.ErrorCodes = new List<string>();
    }
    public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
    {
        _ReCaptchaSecret = reCaptchaSecret;
        _ReCaptchaSiteKey = reCaptchaSiteKey;
        this.ErrorCodes = new List<string>();
    }

    public bool ValidateCaptcha(HttpRequest request)
    {
        var sb = new StringBuilder();
        sb.Append("https://www.google.com/recaptcha/api/siteverify?secret=");
        sb.Append(_ReCaptchaSecret);
        sb.Append("&response=");
        sb.Append(request.Form["g-recaptcha-response"]);

        //client ip address
        sb.Append("&remoteip=");
        sb.Append(GetUserIp(request));

        //make the api call and determine validity
        using (var client = new WebClient())
        {
            var uri = sb.ToString();
            var json = client.DownloadString(uri);
            var serializer = new DataContractJsonSerializer(typeof(RecaptchaApiResponse));
            var ms = new MemoryStream(Encoding.Unicode.GetBytes(json));
            var result = serializer.ReadObject(ms) as RecaptchaApiResponse;

            if (result == null)
            {
                return false;
            }
            else if (result.ErrorCodes != null)
            {
                foreach(var code in result.ErrorCodes)
                {                        
                    this.ErrorCodes.Add(code.ToString());
                }
                return false;
            }
            else if (!result.Success)
            {
                return false;
            }
            else //-- If successfully verified.
            {
                return true;
            }
        }
    }

    //--- To get user IP(Optional)
    private string GetUserIp(HttpRequest request)
    {
        var visitorsIpAddr = string.Empty;

        if (request.ServerVariables["HTTP_X_FORWARDED_FOR"] != null)
        {
            visitorsIpAddr = request.ServerVariables["HTTP_X_FORWARDED_FOR"];
        }
        else if (!string.IsNullOrEmpty(request.UserHostAddress))
        {
            visitorsIpAddr = request.UserHostAddress;
        }
        return visitorsIpAddr;
    }

}
[DataContract]
public class RecaptchaApiResponse
{
    [DataMember(Name = "success")]
    public bool Success;

    [DataMember(Name = "error-codes")]
    public List<string> ErrorCodes;
}

Answer 1

private readonly string _ReCaptchaSecret;
private readonly string _ReCaptchaSiteKey;

private fields are either lowerCamelCase or _lowerCamelCase, both conventions are common.

---

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}
public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
{
    _ReCaptchaSecret = reCaptchaSecret;
    _ReCaptchaSiteKey = reCaptchaSiteKey;
    this.ErrorCodes = new List<string>();
}

Don't repeat work in your constructors: use chaining instead.

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}

public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey) 
    : this (reCaptchaSecret)
{
    _ReCaptchaSiteKey = reCaptchaSiteKey;
}

---

With C# 6, you can also use a property initializer to.. initialize.. your property.

public List<string> ErrorCodes { get; set; }

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}

becomes

public List<string> ErrorCodes { get; set; } = new List<string>();

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
}

---

I would also advise to use a private setter for your ErrorCodes like this:

public List<string> ErrorCodes { get; private set; } = new List<string>();

Likewise, with C# 6, you might as well make it a readonly property:

public List<string> ErrorCodes { get; } = new List<string>();

---

StringBuilders see their most use when you're concatenating strings in a loop. In this scenario I would use a formatted string which turns this:

var sb = new StringBuilder();
sb.Append("https://www.google.com/recaptcha/api/siteverify?secret=");
sb.Append(_ReCaptchaSecret);
sb.Append("&response=");
sb.Append(request.Form["g-recaptcha-response"]);
sb.Append("&remoteip=");
sb.Append(GetUserIp(request));

into this:

var uri = string.Format("https://www.google.com/recaptcha/api/siteverify?secret={0}&response={1}&remoteip={2}",
    _ReCaptchaSecret, 
    request.Form["g-recaptcha-response"], 
    GetUserIp(request));

Or with C# 6:

var uri = $"https://www.google.com/recaptcha/api/siteverify?secret={_ReCaptchaSecret}&response={(request.Form["g-recaptcha-response"])}&remoteip={GetUserIp(request)}";

---

else if (!string.IsNullOrEmpty(request.UserHostAddress))

I prefer string.IsNullOrWhiteSpace because only rarely whitespace characters are considered good input.

Answer 2

I'm not a fan of properties in the form of:

public List<string> ErrorCodes { get; set; }

as someone can do this:

var validator = new ReCaptchaValidator("secretstuff");

validator.ErrorCodes = new List<string> { "Bogus error!" };

or some such. I might code it as such:

private readonly IList<string> _ErrorCodes = new List<string>();

public ReCaptchaValidator(string reCaptchaSecret) : this(reCaptchaSecret, null)
{
}

public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
{
    _ReCaptchaSecret = reCaptchaSecret;
    _ReCaptchaSiteKey = reCaptchaSiteKey;
}

public IEnumerable<string> ErrorCodes
{
    get
    {
        return new ReadOnlyCollection<string>(this._ErrorCodes);
    }
}

Then replace all your use of ErrorCodes with _ErrorCodes within the class.

Answer 3

        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            foreach(var code in result.ErrorCodes)
            {                        
                this.ErrorCodes.Add(code.ToString());
            }
            return false;
        }
        else if (!result.Success)
        {
            return false;
        }
        else //-- If successfully verified.
        {
            return true;
        }  

Well this is ugly.

First the else is redundant, because for each of the other conditions you are returning a value hence the else will be reached only if the previous conditions aren't met.

The last else if is superfluous too, because you have something like this:

if(condition)
{
    return true;
}
else
{ 
    return true;
}  

which should be compacted to return condition;.

This will lead to:

        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            foreach(var code in result.ErrorCodes)
            {                        
                this.ErrorCodes.Add(code.ToString());
            }
            return false;
        }

        return result.Success;

But hey, we can do one better.

• because result.ErrorCodes is a List<string> there is no need to call ToString() on any item of this list.

• because this.ErrorCodes is a List<string> too, we can take advantage of the AddRange() method.

This will lead to:

        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            this.ErrorCodes.AddRange(result.ErrorCodes);
            return false;
        }

        return result.Success;