[Login] Enable optional reCAPTCHA on Request Account page #6344

johnsaigle · 2020-04-08T20:49:42Z

Brief summary of changes

Adds a reCAPTCHA validation feature to the existing request account page.

reCAPTCHA is only active when a private key has been configured on the server. Otherwise it will be ignored.

This will probably need to be reworked alongside #5828 in the future.

Have you updated related documentation?

Testing instructions (if applicable)

Enable reCAPTCHA by following the Google reCAPTCHA instructions for version 2.
Update the config settings in LORIS as described in the new wiki document
Try requesting an account with and without submitting a reCAPTCHA and make sure it seems to work well.

Link(s) to related issue(s)

Resolves [login] Recaptcha not displaying #6244

johnsaigle · 2020-04-08T20:51:07Z

modules/login/php/requestaccount.class.inc

+        $this->tpl_data['captcha_key'] = \NDB_Factory::singleton()
+             ->config()
+             ->getSetting('reCAPTCHAPublic');
+        $this->tpl_data['nonce'] = base64_encode(openssl_random_pseudo_bytes(16));


This is required as a security measure to avoid making our Content Security Policy weaker. Detailed discussion here. google/recaptcha#107

johnsaigle · 2020-04-08T20:51:44Z

modules/login/php/requestaccount.class.inc

+                    $_SERVER['REMOTE_ADDR']
+                );
+            $success = $resp->isSuccess();
+            // FIXME This leverages the error handling for the "site" text field.


I couldn't figure out how to get an error code to show up under the reCAPTCHA part so this is a bit hack-y but is functioning. I'm open to suggestions

@johnsaigle maybe $resp returns null if error? I'm wondering what happens when an error is thrown.

Oh sorry I didn't add the new file to Git. 😅

johnsaigle · 2020-04-08T20:52:18Z

modules/login/php/requestaccount.class.inc

@@ -211,18 +242,6 @@ class RequestAccount extends \NDB_Form
            }
        }

-        // Verify reCAPTCHA


This is moved to _validate instead of _process. It doesn't make sense to insert a user into the DB if they failed the reCAPTCHA.

johnsaigle · 2020-04-08T20:52:58Z

modules/login/templates/form_requestaccount.tpl

-              <b class="text-danger">{$error_message['captcha']}</b>
-            </span>
-          </div>
+            <script src="https://www.google.com/recaptcha/api.js?render={$captcha_key}&onload=onloadCallback nonce={$nonce}"></script>


This should probably be refactored in a more generic way so that we can use reCAPTCHA in other places if we want to. That may be out of scope for now, plus I'm not really sure the best approach. Open for suggestions.

I'm guessing a new react component will be made. 🙂

johnsaigle · 2020-04-08T20:55:23Z

modules/login/test/Request_Account_test_plan.md

 3. Verify that verification code is enforced (if and only if re-captcha has been activated by the project)
-4. Verify that new verification code loads on every refresh of the page


This refers to a very very old custom captcha that we deleted because it was insecure. #3645

maltheism · 2020-04-08T21:01:24Z

Hey @johnsaigle, I'll be happy to test this PR and I see it's set as work in progress. Let me know when I should manually test it.

johnsaigle · 2020-04-08T21:02:25Z

You can manually test it now. Thanks!

It's in draft mode only because I have a couple of hacks in the code that could be done better.

It's totally functional.

modules/login/php/requestaccount.class.inc

maltheism · 2020-04-08T21:12:19Z

@johnsaigle probably good to update somewhere that we're using v3 of reCAPTCHA.

maltheism · 2020-04-08T21:16:55Z

@johnsaigle frontend is giving me this output in console:
Refused to frame 'https://www.google.com/' because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'frame-src' was not explicitly set, so 'default-src' is used as a fallback.
The reCAPTCHA window is greyed out because of it. I think the nonce has to be included in the content-security-policy header of the NDB_Client.class.inc? I'm unsure though.

johnsaigle · 2020-04-14T14:49:51Z

@maltheism

Update the config settings in LORIS as described in the new wiki document

Did you do this step?

You need to change the CSP. I described how in the wiki document in this PR.

Also this is using v2, not v3. I couldn't get v3 to work. This is mentioned in that document as well.

maltheism · 2020-04-14T15:20:43Z

@johnsaigle I'm not seeing the wiki readme. I was using a v3 key so I'll need to fix that as well.

johnsaigle · 2020-04-14T15:26:52Z

docs/wiki/99 - Developers/reCAPTCHA.md

+corresponding values from your Google reCAPTCHA admin account.
+This can also be configured via the Configuration module by modifying the values found in the "API Keys" heading.
+
+* `CSPAdditionalHeaders` should include "frame-src www.google.com;" in order


@maltheism These are the instructions you need. Sorry for the confusion

@johnsaigle maybe not for this PR but I'm a little confused why the way of going about this is by editing the database to include frame-src www.google.com;. I almost think there should just be a check in the code to see if reCAPTCHAPRIVATE has been added to the config.xml or admin configuration and if so add the frame-src www.google.com; to the content security policy. I would think that would be best for having it be hands free for users.

Yeah that would probably be a good enhancement.

maltheism · 2020-04-14T16:31:31Z

Passed manual testing for me.

modules/login/php/requestaccount.class.inc

modules/login/templates/form_requestaccount.tpl

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

driusan · 2020-04-21T18:29:08Z

mkdocs.yml

@@ -32,4 +32,5 @@ nav:
        - 'SQL Dictionary': 'wiki/99 - Developers/SQL Dictionary.md'
        - 'Automated Testing': 'wiki/99 - Developers/Automated Testing.md'
        - 'Style Guide (for help text)': 'HelpStyleGuide.md'
+        - 'Enabling reCAPTCHA': 'wiki/99 - Developers/reCAPTCHA.md'


Why is this in the developers section? Shouldn't it be in the installation and configuration section? It's more for prod than devs..

Yeah good point

modules/login/templates/form_requestaccount.tpl

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

Adds a reCAPTCHA validation feature to the existing request account page. reCAPTCHA is only active when a private key has been configured on the server. Otherwise it will be ignored.

[Login] Enable optional reCAPTCHA on Request Account page

db25040

johnsaigle requested a review from ridz1208 April 8, 2020 20:49

johnsaigle commented Apr 8, 2020

View reviewed changes

johnsaigle mentioned this pull request Apr 8, 2020

Create script to validate server configuration #5965

Merged

8 tasks

johnsaigle commented Apr 8, 2020

View reviewed changes

modules/login/php/requestaccount.class.inc Outdated Show resolved Hide resolved

modules/login/php/requestaccount.class.inc Outdated Show resolved Hide resolved

modules/login/php/requestaccount.class.inc Outdated Show resolved Hide resolved

remove unnecessary variable

e0acc0b

johnsaigle marked this pull request as ready for review April 8, 2020 21:05

johnsaigle added the Needs Code Formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) label Apr 14, 2020

add wiki file

5fbc239

johnsaigle commented Apr 14, 2020

View reviewed changes

maltheism added the Passed Manual Tests PR has undergone proper testing by at least one peer label Apr 14, 2020

maltheism approved these changes Apr 14, 2020

View reviewed changes

ridz1208 linked an issue Apr 15, 2020 that may be closed by this pull request

[login] Recaptcha not displaying #6244

Closed

cmadjar assigned johnsaigle Apr 15, 2020

static analysis errors

589940d

johnsaigle removed the Needs Code Formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) label Apr 21, 2020

johnsaigle requested a review from driusan April 21, 2020 17:19

driusan requested changes Apr 21, 2020

View reviewed changes

modules/login/php/requestaccount.class.inc Outdated Show resolved Hide resolved

modules/login/php/requestaccount.class.inc Outdated Show resolved Hide resolved

modules/login/templates/form_requestaccount.tpl Outdated Show resolved Hide resolved

Apply Dave's suggestions from code review

7b76915

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

johnsaigle requested a review from driusan April 21, 2020 18:26

driusan reviewed Apr 21, 2020

View reviewed changes

johnsaigle and others added 2 commits April 21, 2020 14:52

Update modules/login/templates/form_requestaccount.tpl

39b4b09

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

move doc to different wiki folder

e277ad4

driusan merged commit cf7a8aa into aces:23.0-release Apr 21, 2020

ridz1208 added this to the 23.0.0 milestone Apr 27, 2020

johnsaigle mentioned this pull request Jun 3, 2020

reCapcha Plan: Implement Test and Fix with latest updates. #4450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Login] Enable optional reCAPTCHA on Request Account page #6344

[Login] Enable optional reCAPTCHA on Request Account page #6344

johnsaigle commented Apr 8, 2020 •

edited

Loading

johnsaigle Apr 8, 2020

johnsaigle Apr 8, 2020

maltheism Apr 8, 2020

johnsaigle Apr 14, 2020

johnsaigle Apr 8, 2020

johnsaigle Apr 8, 2020

maltheism Apr 8, 2020

johnsaigle Apr 8, 2020

maltheism commented Apr 8, 2020 •

edited

Loading

johnsaigle commented Apr 8, 2020

maltheism commented Apr 8, 2020

maltheism commented Apr 8, 2020

johnsaigle commented Apr 14, 2020 •

edited

Loading

maltheism commented Apr 14, 2020

johnsaigle Apr 14, 2020

maltheism Apr 14, 2020

johnsaigle Apr 14, 2020

maltheism commented Apr 14, 2020

driusan Apr 21, 2020

johnsaigle Apr 21, 2020

		3. Verify that verification code is enforced (if and only if re-captcha has been activated by the project)
		4. Verify that new verification code loads on every refresh of the page

[Login] Enable optional reCAPTCHA on Request Account page #6344

[Login] Enable optional reCAPTCHA on Request Account page #6344

Conversation

johnsaigle commented Apr 8, 2020 • edited Loading

Brief summary of changes

Testing instructions (if applicable)

Link(s) to related issue(s)

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

maltheism commented Apr 8, 2020 • edited Loading

johnsaigle commented Apr 8, 2020

maltheism commented Apr 8, 2020

maltheism commented Apr 8, 2020

johnsaigle commented Apr 14, 2020 • edited Loading

maltheism commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltheism commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Apr 8, 2020 •

edited

Loading

maltheism commented Apr 8, 2020 •

edited

Loading

johnsaigle commented Apr 14, 2020 •

edited

Loading