10
\$\begingroup\$

This is the third iteration of my reCAPTCHA validator. Iteration II can be found at: Google reCAPTCHA Validator: Iteration II

It does almost everything required to do reCAPTCHA validation on anything .NET.

I removed all the XML docs to shorten it up a bit.

GitHub commit of the version in this thread (with all the docs added that I omitted here): ReCaptchaValidator.cs

The only comments I do not wish to hear of are any comments relating to my use of fully-qualifying static/const members. I do this primarily to remind myself of where they originate.

using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Linq;
using System.Net;
using System.Web;
using System.Web.Script.Serialization;

namespace Evbpc.Framework.Integrations.Google
{
    public class ReCaptchaValidator
    {
        private const string _headScriptInclude = "<script src='https://www.google.com/recaptcha/api.js'></script>";
        private const string _bodyDivInclude = "<div class=\"g-recaptcha %EXTRACLASSES%\" data-sitekey=\"%SITEKEY%\"></div>";
        private const string _reCaptchaFormCode = "g-recaptcha-response";
        private const string _googleApiEndpoint = "https://www.google.com/recaptcha/api/siteverify";

        private readonly string _reCaptchaSecret;
        private readonly string _reCaptchaSiteKey;
        private readonly List<string> _extraClasses = new List<string>();

        public string HeadScriptInclude => ReCaptchaValidator._headScriptInclude;
        public List<string> ExtraClasses => _extraClasses;
        public string BodyDivInclude => GetBodyDivContent(this.ExtraClasses);

        public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
        {
            _reCaptchaSecret = reCaptchaSecret;
            _reCaptchaSiteKey = reCaptchaSiteKey;
        }

        public ReCaptchaResponse Validate(NameValueCollection form, string remoteIp = null)
        {
            string reCaptchaSecret = _reCaptchaSecret;
            string reCaptchaResponse = form[ReCaptchaValidator._reCaptchaFormCode];

            using (WebClient client = new WebClient())
            {
                NameValueCollection postParameters = new NameValueCollection() { { "secret", reCaptchaSecret }, { "response", reCaptchaResponse } };

                if (remoteIp != null)
                {
                    postParameters.Add("remoteip", remoteIp);
                }

                byte[] response = client.UploadValues(ReCaptchaValidator._googleApiEndpoint, postParameters);

                string reCaptchaResult = System.Text.Encoding.UTF8.GetString(response);

                ReCaptchaResponse result = new ReCaptchaResponse();

                if (result.ParseJson(reCaptchaResult))
                {
                    return result;
                }

                return null;
            }
        }

        public string GetBodyDivContent(List<string> extraClasses)
        {
            string result = ReCaptchaValidator._bodyDivInclude;

            result = result.Replace("%SITEKEY%", _reCaptchaSiteKey);

            if (extraClasses != null)
            {
                result = result.Replace("%EXTRACLASSES%", string.Join(" ", extraClasses));
            }

            return result;
        }
    }

    public class ReCaptchaResponse
    {
        private bool _success = false;
        private ReCaptchaErrors _errors = ReCaptchaErrors.None;

        public bool Success => _success; 
        public ReCaptchaErrors Errors => _errors;

        public bool ParseJson(string jsonResponse)
        {
            JavaScriptSerializer jss = new JavaScriptSerializer();
            dynamic deserializedJson = jss.DeserializeObject(jsonResponse);

            if (deserializedJson.ContainsKey("success"))
            {
                _success = deserializedJson["success"];
                _errors = ReCaptchaErrors.None;

                if (deserializedJson.ContainsKey("error-codes"))
                {
                    foreach (string error in deserializedJson["error-codes"])
                    {
                        // Our `ReCaptchaErrors` enum contains the exact same strings as the returned `error` text would be, with the following transformations:
                        // 1. The words are transformed to PascalCase;
                        // 2. The dashes are stripped;
                        string[] errorWords = error.Split('-');

                        string errorEnumName = "";
                        foreach (string errorWord in errorWords)
                        {
                            errorEnumName += errorWord[0].ToString().ToUpper() + errorWord.Substring(1);
                        }

                        if (Enum.IsDefined(typeof(ReCaptchaErrors), errorEnumName))
                        {
                            _errors = _errors | (ReCaptchaErrors)Enum.Parse(typeof(ReCaptchaErrors), errorEnumName);
                        }
                        else
                        {
                            return false;
                        }
                    }
                }
            }
            else
            {
                return false;
            }

            return true;
        }
    }

    [Flags]
    public enum ReCaptchaErrors
    {
        None = 0x00,
        MissingInputSecret = 1 << 0,
        InvalidInputSecret = 1 << 1,
        MissingInputResponse = 1 << 2,
        InvalidInputResponse = 1 << 3,
    }
}

Yes, this is the first question I have asked here with all the braces included.

This is part of an open-source library I have which, as usual, anyone is free to use.

\$\endgroup\$
6
\$\begingroup\$

The strings "success" and "error-codes" should be const string.


Why is the construction of postParameters inside using (WebClient client = new WebClient())? On that same topic: considering client isn't used after it has filled response, why keep it open? Basically, only response = client.UploadValues(ReCaptchaValidator._googleApiEndpoint, postParameters); should be inside using (WebClient client = new WebClient()) (with byte[] response defined outside of it).

You also don't need the empty argument at the end of NameValueCollection().


Can remoteIp be an empty string? Otherwise, why not use string.IsNullOrEmpty instead of if (remoteIp != null)?


There's no need to do this:

string result = ReCaptchaValidator._bodyDivInclude;
result = result.Replace("%SITEKEY%", _reCaptchaSiteKey);

Just do it in one line:

string result = ReCaptchaValidator._bodyDivInclude.Replace("%SITEKEY%", _reCaptchaSiteKey);

I'm not a fan of an enum with a plural as a name: ReCaptchaErrors. Especially since you then named the field _errors, which suggests a collection. Same for Errors.


Why do you use JavaScriptSerializer and not Json.NET?


You can reduce the indentation by re-thinkign your if-else blocks:

if (!deserializedJson.ContainsKey("success"))
{
    return false;
}

_success = deserializedJson["success"];
_errors = ReCaptchaErrors.None;

if (!deserializedJson.ContainsKey("error-codes"))
{
    return true;
}

It's customary to use string.Empty instead of "": string errorEnumName = "";.


Move errorWord[0].ToString().ToUpper() + errorWord.Substring(1); to a method called Capitalize and rewrite it slightly:

private string Capitalize(string s)
{
    if (string.IsNullOrEmpty(s))
    {
        return string.Empty;
    }
    return char.ToUpper(s[0]) + s.Substring(1);
}

That way your for loop can become:

string errorEnumName = errorWords.Aggregate(string.Empty, (current, errorWord) => current + Capitalize(errorWord));

I would consider converting deserializedJson to a case insensitive Dictionary<K, V> to avoid the foreach. Also, that way you can use TryGetValue.

\$\endgroup\$
2
  • \$\begingroup\$ I ended up renaming Capitalize to ToPascalCase, made it an extension method, and put it in a different class. :) \$\endgroup\$ – Der Kommissar Sep 28 '15 at 14:33
  • \$\begingroup\$ Re: the plural enum name - the Enumeration Type Naming Guidelines suggest this is correct, since it is a flags enum. \$\endgroup\$ – Dan Lyons Sep 28 '15 at 18:38
3
\$\begingroup\$

Just one quick comment:

ReCaptchaResponse result = new ReCaptchaResponse();

if (result.ParseJson(reCaptchaResult))
{
    return result;
}
return null;

This would work better as static method following the familiar TryParse pattern:

ReCaptchaResponse result;
if (ReCaptchaResponse.TryParseJson(reCaptchaResult, out result))
{
    return result;
}
return null;
\$\endgroup\$
2
\$\begingroup\$

Some changes I made (after noticing a few issues with it):

  • ExtraClasses has no need to be a field and a property; replaced both with public List<string> ExtraClasses { get; } = new List<string>();.
  • Since GetBodyDivContent is not static, I made removed extraClasses from the parameter, and made it directly used in the method.
  • I extracted a lot of Validate out to other protected methods.
  • I replaced ParseJson with TryParseJson as per RobH's answer.

Version as of this answer: ReCaptchaValidator.cs

\$\endgroup\$
2
  • \$\begingroup\$ That looks a lot nicer, yes. Invalidates plenty of my review, though. ;-) \$\endgroup\$ – BCdotWEB Sep 28 '15 at 14:25
  • \$\begingroup\$ @BCdotWEB Although I am making substantial changes as a result of your review still. :) \$\endgroup\$ – Der Kommissar Sep 28 '15 at 14:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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