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;
}

• Der Kommissar: I never thought to use DataContracts for mine, that's a good write!

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.

• Malachi: I could not remember the chaining like this for some reason. I just read about it too! thank you!
• Malachi: I was able to do this private readonly List<string> _errorCodes = new List<string>(); does that mean that I am running C# 6?
• Jeroen Vannevel: No, not necessarily. That's a field, not a property. Fields can be instantiated inline, properties only can since C# 6.
• Malachi: you need to escape your quotes in the interpolated string
• Malachi: it appears that you need to have that expression inside of parenthesis. like so --> {(request.Form["g-recaptcha-response"])} via this Stack Overflow Answer

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;