New users might be registered in a massive way by a robot

[davide-negretti]

References

• Fixes New users might be registered in a massive way by a robot DSpace#8256
• Requires New users might be registered in a massive way by a robot DSpace#8482

Description

The registration page now uses Google reCAPTCHA in order to prevent bots to register to DSpace.
Both v2 (invisible/checkbox) and v3 (invisible) versions of reCAPTCHA are supported.
Cookie policy has been made compliant

Instructions for Reviewers

Add to dspace.cfg one of the following configurations:

registration.verification.enabled=true

# FOR reCAPTCHA v3
google.recaptcha.version = v3
google.recaptcha.key.site = 6LfmfEsgAAAAACNqQ0aHqJa0HOHcUsvv2OCiEbV4
google.recaptcha.key.secret = 6LfmfEsgAAAAAJIiSifDW2sElGtK28IjRN0Nc9tn
google.recaptcha.site-verify = https://www.google.com/recaptcha/api/siteverify
google.recaptcha.key.threshold = 0.5
google.recaptcha.mode = invisible

# FOR reCAPTCHA v2
google.recaptcha.version = v2
google.recaptcha.mode = invisible
google.recaptcha.key.site=6LcBs2IhAAAAADsBGOpHLU2XiGjnI85_gqcygIcB
google.recaptcha.key.secret=6LcBs2IhAAAAADw6X0H91PiqKfBOejSUTPLdkn7-

# FOR reCAPTCHA v2 WITH CHECKBOX
google.recaptcha.version = v2
google.recaptcha.mode = checkbox
google.recaptcha.key.site=6LcQrmIhAAAAAJSk4fGB0Mqiqk73SyQkq1NLnQ0_
google.recaptcha.key.secret=6LcQrmIhAAAAAGHIM7o3lgdgsaPM3dz3iRBeuKef

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR doesn't introduce circular dependencies
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[lgtm-com]

This pull request introduces 1 alert when merging ac34a78 into e7dc5f8 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[tdonohue]

@davide-negretti or @atarix83 : This has a brand new merge conflict (possibly caused by the recently merged Google Analytics PR). Could either of you quickly rebase this? Thanks!

[lgtm-com]

This pull request fixes 2 alerts when merging 88d5f02 into 6f9d310 - view on LGTM.com

fixed alerts:

• 2 for Unused variable, import, function or class

[davide-negretti]

@tdonohue I aligned the code to main branch, and I fixed the problem you mentioned on DSpace/DSpace#8482

I still have to fix some tests

[tdonohue]

@davide-negretti : Thanks! This is working for me. Retested today with both reCaptcha v2 and v3 and they both work. But it looks like it is still failing tests. I'm basically a +1 , but the tests need to be fixed obviously

[davide-negretti]

@tdonohue I think that the failing test is caused by #1851.

  ItemPageAbstractFieldComponent
    ✖ should display display the correct metadata value

The test seems to fail randomly:

• it passed on GitHub build for Add support for line breaks markdown and mathjax in metadata #1851
• it both passed and failed on my machine for main branch (aligned with Add support for line breaks markdown and mathjax in metadata #1851)
• it both passed and failed on my machine for this PR
• it failed on GitHub with Node 16 only for this PR

I cannot find any way this PR could interfere with that component. I think that this PR could me merged, and the failing test should be fixed on main branch.

[tdonohue]

👍 Thanks @davide-negretti! Re-submitting my prior review as a +1. As I said above, this works for me now with reCaptcha v2 and v3

You are correct in your analysis of the randomly failing test. I see that work has begun to make this randomly failing test more stable in #1872. This PR can move forward as that test failures are definitely random & not caused by this PR.

[benbosman]

When no captcha is configured, it seems that /server/api/config/properties/registration.verification.enabled is not requested since the /register page still displays:

In order to register you must accept the Registration and Password recovery (Google reCaptcha) cookies.

Open cookie settings

Can the value of /server/api/config/properties/registration.verification.enabled be checked first, to avoid site that don't use the captcha would need users to still accept it

[davide-negretti]

@benbosman I added the missing check and fixed the "Register" button accordingly

[lgtm-com]

This pull request fixes 2 alerts when merging 2e4b96b into 458df45 - view on LGTM.com

fixed alerts:

• 2 for Unused variable, import, function or class

[tdonohue]

@davide-negretti : I retested this today & verified that the bug reported by @benbosman has been fixed. However, during my testing, I noticed a somewhat related issue... when reCaptcha is disabled, it's still listed in the Klaro cookie popup.

Here's how to reproduce it:

1. Disable reCaptcha (default setting)
2. Open up UI in Incognito mode. Check the Klaro popup and you'll still see reCaptcha listed:

This is not a major issue, but obviously it'd be best to not ask for permissions that we don't need. So, ideally we'd not show the "Registration and Password recovery" section when reCaptcha is disabled on the backend.

Beyond that, all the actual reCaptcha functionality is still working. Re-tested today with v2 and v3 and didn't see any issues.

[benbosman]

I've also reviewed and re-tested, and don't see any issues apart from what @tdonohue mentioned

[davide-negretti]

Hi @tdonohue @benbosman, I fixed the Klaro issue

[lgtm-com]

This pull request fixes 2 alerts when merging 3b7a830 into 9305705 - view on LGTM.com

fixed alerts:

• 2 for Unused variable, import, function or class

[tdonohue]

👍 Thanks @davide-negretti ! I've verified the Klaro issue is now fixed. I've also retested that v2 and v3 reCaptchas both work. Looks good now. As soon as tests pass (I just restarted what looks like a random failure), I'll merge this as both @benbosman and I have approved.