Support validation when value contains response #31

akrabat · 2017-02-22T10:10:32Z

If the user passes in the response value directly to isValid(), e.g. by taking the g-recaptcha-response value directly, then this should work!

Fixes #30

If the user passes in the response value directly to isValid(), e.g. by taking the `g-recaptcha-response` value directly, then this should work! Fixes zendframework#30

akrabat · 2017-02-22T10:40:49Z

Note that we set the SSL Transport to TLS in the tests specifically because Travis uses PHP 5.6.5. From PHP 5.6.7, the Socket adapter uses TLS by default, but in 5.6.5, it uses SSLv3, which Google doesn't accept.

Ocramius · 2017-02-22T10:43:26Z

phpunit.xml.dist

@@ -27,6 +27,10 @@
        <!-- Enable this if you have installed ZendService\ReCaptcha on the
             include_path or via Composer. -->
        <env name="TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT" value="false" />
+        <!-- Change these if you want to use your own keys -->


Are these real values, or mocked up ones? If we're using real values, these should be encrypted in the travis-ci config, or else somebody else may abuse them (unless google has "test values")

They are the official test ones. See https://developers.google.com/recaptcha/docs/faq#id-like-to-run-automated-tests-with-recaptcha-v2-what-should-i-do

Would then:

add the link to the comment

describe what they represent, since they are fake values

Ocramius · 2017-02-22T10:44:47Z

src/ReCaptcha.php

@@ -260,8 +260,11 @@ public function isValid($value, $context = null)
        }

        $service = $this->getService();
+        if (array_key_exists($value, $context)) {


Nothing guarantees that $value is a string or integer here.

Ocramius · 2017-02-22T10:45:47Z

src/ReCaptcha.php

@@ -260,8 +260,11 @@ public function isValid($value, $context = null)
        }

        $service = $this->getService();
+        if (array_key_exists($value, $context)) {
+            $value = $context[$value];


Instead of overriding $value, do use an } else { and call $res = $service->verify(...) with a different parameters. This avoids having a semantic change in the meaning of $value

Ocramius · 2017-02-22T10:46:27Z

test/ReCaptchaTest.php

+    public function testValidationForDifferentElementName()
+    {
+        $captcha = new ReCaptcha([
+            'site_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SITE_KEY'),


Should probably skip these tests if either of these variables are not defined (see comment above about committed secrets)

setUp() has a guard for the 'TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT environment variable. Do we need a separate one for these keys?

I think no.

Agreed with @akrabat , though I've noted a problem with the "skip" message in setUp().

Ocramius · 2017-02-22T10:46:30Z

test/ReCaptchaTest.php

+    public function testValidationForResponseElementName()
+    {
+        $captcha = new ReCaptcha([
+            'site_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SITE_KEY'),


Should probably skip these tests if either of these variables are not defined (see comment above about committed secrets)

Ocramius · 2017-02-22T10:46:49Z

test/ReCaptchaTest.php

+            'site_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SITE_KEY'),
+            'secret_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SECRET_KEY'),
+        ]);
+        $captcha->getService()->setIp('127.0.0.1');


Avoid repeating a getService() call

Ocramius · 2017-02-22T10:46:53Z

test/ReCaptchaTest.php

+            'site_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SITE_KEY'),
+            'secret_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SECRET_KEY'),
+        ]);
+        $captcha->getService()->setIp('127.0.0.1');


Avoid repeating a getService() call

Ocramius · 2017-02-22T10:47:17Z

test/ReCaptchaTest.php

+        $captcha->getService()->setIp('127.0.0.1');
+        $captcha->getService()->setHttpClient($this->getHttpClient());
+
+        $response = getenv('TESTS_ZEND_SERVICE_RECAPTCHA_RESPONSE');


Ah, so I assume the environment vars are actual mock values?

Ocramius · 2017-02-22T10:47:27Z

test/ReCaptchaTest.php

+        $this->assertTrue($captcha->isValid($value, $context));
+    }
+
+    private function getHttpClient()


Return type in docblock, please

Ocramius · 2017-02-22T10:48:06Z

test/ReCaptchaTest.php

+        $socket->setOptions([
+            'ssltransport' => 'tls',
+        ]);
+        return new HttpClient(null, [


This indenting is not PSR-2 compliant

Yes it is!

$ vendor/bin/phpcs test/ReCaptchaTest.php . Time: 101ms; Memory: 6Mb

Definitely PSR-2 compliant; single argument spanning multiple lines. If both arguments were arrays, then you would need to start each on a new line, but because it's only one, can be done this way.

This avoids having a semantic change in the meaning of $value.

This makes sure that we can use it with array_key_exists.

akrabat · 2017-02-22T11:41:49Z

Updated as per @Ocramius' comments.

Travis passed: https://travis-ci.org/zendframework/zend-captcha/builds/204158887

weierophinney

Looks good! The only concern I have is that the setUp() validation of the TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT constant emits a misleading message when the constant is missing. I realize that's from a previous commit somewhere, but might as well change it now, since these tests rely on it.

Once fixed, feel free to merge!

weierophinney · 2017-02-22T15:08:25Z

test/ReCaptchaTest.php

@@ -28,14 +30,6 @@ public function setUp()
        if (! getenv('TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT')) {
            $this->markTestSkipped('Enable TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT to test PDF render');


I think this message needs to be different, as recaptcha has nothing to do with PDF...

weierophinney · 2017-02-22T15:08:46Z

test/ReCaptchaTest.php

+    public function testValidationForDifferentElementName()
+    {
+        $captcha = new ReCaptcha([
+            'site_key' => getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SITE_KEY'),


Agreed with @akrabat , though I've noted a problem with the "skip" message in setUp().

weierophinney · 2017-02-22T15:10:08Z

test/ReCaptchaTest.php

+        $socket->setOptions([
+            'ssltransport' => 'tls',
+        ]);
+        return new HttpClient(null, [


Definitely PSR-2 compliant; single argument spanning multiple lines. If both arguments were arrays, then you would need to start each on a new line, but because it's only one, can be done this way.

Close #31 Fix #30

Support validation when value contains response

6076a48

If the user passes in the response value directly to isValid(), e.g. by taking the `g-recaptcha-response` value directly, then this should work! Fixes zendframework#30

akrabat force-pushed the element-name-g-recaptcha-response branch from 0f669fd to 6076a48 Compare February 22, 2017 10:11

Use TLS transport for Travis PHP 5.6

308b949

akrabat force-pushed the element-name-g-recaptcha-response branch from 8a5e745 to 308b949 Compare February 22, 2017 10:33

akrabat requested review from Ocramius and weierophinney February 22, 2017 10:39

Ocramius suggested changes Feb 22, 2017

View reviewed changes

Ocramius assigned akrabat Feb 22, 2017

Ocramius added awaiting author updates bug labels Feb 22, 2017

akrabat added 5 commits February 22, 2017 11:22

Document the test keys in oh-unit.xml.dist

bd6f09d

Don’t repurpose value when extracting response from context

ef127f3

This avoids having a semantic change in the meaning of $value.

Ensure that value is a string or integer

b9be365

This makes sure that we can use it with array_key_exists.

Add return type to docblock in ReCaptchTest

98cb25b

Avoid multiple calls to getService()

7594b57

akrabat removed the awaiting author updates label Feb 22, 2017

weierophinney suggested changes Feb 22, 2017

View reviewed changes

akrabat added this to the 2.7.1 milestone Feb 22, 2017

Fix skipped tests message

64346d0

akrabat force-pushed the element-name-g-recaptcha-response branch from 1286ce9 to 64346d0 Compare February 23, 2017 07:46

akrabat merged commit 64346d0 into zendframework:master Feb 23, 2017

akrabat added a commit that referenced this pull request Feb 23, 2017

Merge branch 'element-name-g-recaptcha-response'

97a3900

Close #31 Fix #30

akrabat deleted the element-name-g-recaptcha-response branch February 23, 2017 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support validation when value contains response #31

Support validation when value contains response #31

akrabat commented Feb 22, 2017

akrabat commented Feb 22, 2017

Ocramius Feb 22, 2017

akrabat Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

akrabat Feb 22, 2017 •

edited

Loading

weierophinney Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

Ocramius Feb 22, 2017

akrabat Feb 22, 2017

weierophinney Feb 22, 2017

akrabat commented Feb 22, 2017 •

edited

Loading

weierophinney left a comment

weierophinney Feb 22, 2017

weierophinney Feb 22, 2017

weierophinney Feb 22, 2017

		@@ -28,14 +30,6 @@ public function setUp()
		if (! getenv('TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT')) {
		$this->markTestSkipped('Enable TESTS_ZEND_CAPTCHA_RECAPTCHA_SUPPORT to test PDF render');

Support validation when value contains response #31

Support validation when value contains response #31

Conversation

akrabat commented Feb 22, 2017

akrabat commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrabat Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrabat commented Feb 22, 2017 • edited Loading

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrabat Feb 22, 2017 •

edited

Loading

akrabat commented Feb 22, 2017 •

edited

Loading