karser / KarserRecaptcha3Bundle

Google ReCAPTCHA v3 for Symfony
MIT License
158 stars 21 forks source link

Reveal errors from response #26

Closed karser closed 3 years ago

karser commented 3 years ago

Fixes #25

Installation

composer require "karser/karser-recaptcha3-bundle:dev-error-codes-based-validation"

Usage:

karser_recaptcha3:
    site_key: 'key'
    secret_key: 'secret'
    enabled: true
    score_threshold: 1
    reveal_errors_from_response: true
sabat24 commented 3 years ago

I tested it and the code per se works good. However, I noticed two things:

  1. You rely on ReCaptcha project so I think that you may add also their own code errors in your validator's getErrorMessage method. Something like that:

    case ReCaptcha::E_CONNECTION_FAILED:
    return $constraint->messageConnectionFailed;

    I made a test simulating connection problem and I received Your computer or network may be sending automated queries because that lib also validates the response and adds it's own errors which aren't recognized by that method. I can help and send you a list with additional codes from that project with messages.

  2. If there is no error code implemented in getErrorMessage method, you will return a default message. Which is basically correct, but leads to two situations: a) Default message is a little bit confusing. I spent a while at first time to debug that it's not a problem with sending auto-queries from my network. I would change it to something more general like: problem with verifying reCapcha appeared. b) If you receive 2 not implemented error codes you will get 2 same default messages.

It may be changed by something like that:

// validateCaptcha
$unknownErrorCode = false;
foreach ($response->getErrorCodes() as $errorCode) {
    $message = $this->getErrorMessage($errorCode, $constraint);
    if ($message === false) {
        $unknownErrorCode = true;
        continue;
    }
    $this->buildViolation($message, $value);
}
if ($unknownErrorCode === true) {
    $this->buildViolation($constraint->message, $value);
}

// getErrorMessage - return false instead of default message
...
default:
    return false;
karser commented 3 years ago

Thanks @sabat24, I didn't notice the constants in ReCaptcha class. Hopefully they don't change them often.

If there is no error code implemented in getErrorMessage method, you will return a default message. Which is basically correct

I took a look with fresh eyes and realized that you only need reveal_errors_from_response: true for development. There is no need to reveal the actual fail reason to the website user. So you would rather see the raw error-code instead of the default message. Makes sense?

Please, take a look at the changes.

sabat24 commented 3 years ago

To be honest the raw codes are enough for development and production as well. You only need them to log problems with captcha or even you can display them directly to user if you need. He will be able to provide you the error code which he received. He doesn't need to understand what the code means and why exactly captcha didn't work.

$formBuilder->add('captcha', Recaptcha3Type::class, [
                         'constraints' => new Recaptcha3(['message' => 'There were problems with your captcha. Please try again or contact with support and provide following code(s): {{ value }}']),
                     ])

For me such message is very enough. value is the imploded array of any error codes that were received.

So if you need the codes you should put a variable into message string. If you do not need the codes, you can just use a string without any variable.

However the feature when validator returns all nice messages can be also fine.

karser commented 3 years ago

@sabat24 that's good idea.

It's gona be the {{ errorCodes }} variable:

$formBuilder->add('captcha', Recaptcha3Type::class, [
    'constraints' => new Recaptcha3(['message' => 'There were problems with your captcha. Please try again or contact with support and provide following code(s): {{ errorCodes }}']),
])
karser commented 3 years ago

Looks like it's bad idea to rely on constants. For some ReCaptcha versions it's gona fail https://travis-ci.org/github/karser/KarserRecaptcha3Bundle/jobs/721408843 I'll revert to string codes.

sabat24 commented 3 years ago

Didn't expect such error. And yes it would be much safer solution.