ReCAPTCHA on User Lost Password Page #11214
Conversation
| $this->addJavaScript( | ||
| 'recaptcha', | ||
| 'https://www.recaptcha.net/recaptcha/api.js?hl=' . substr(AppLocale::getLocale(),0,2), | ||
| [ | ||
| 'contexts' => ['frontend-user-register', 'frontend-user-registerUser'], | ||
| 'contexts' => ['frontend-user-register', 'frontend-user-registerUser', 'frontend-login-lostPassword'], |
There was a problem hiding this comment.
This should be a conditional include, see this for reference: https://github.com/pkp/pkp-lib/pull/6541/files#diff-8100c829b3c8e2b0f5bbf2d00cbc29f5b67dfa0b7183132bfa8694c7d6e476a7R202-R218
There was a problem hiding this comment.
Gotcha. I'll send a new commit with these updates. Thanks @jonasraoni !
pages/login/LoginHandler.inc.php
Outdated
| $captchaEnabled = Config::getVar('captcha', 'captcha_on_password_reset') && Config::getVar('captcha', 'recaptcha'); | ||
| if ($captchaEnabled) { | ||
| import('lib.pkp.classes.form.Form'); | ||
| $form = new Form(''); |
There was a problem hiding this comment.
| $form = new Form(''); | |
| $form = new Form(); |
pages/login/LoginHandler.inc.php
Outdated
| $email = $request->getUserVar('email'); | ||
| $userDao = DAORegistry::getDAO('UserDAO'); /** @var UserDAO $userDao */ | ||
| $user = $userDao->getUserByEmail($email); | ||
|
|
||
| $rateLimitingEnabled = true; |
There was a problem hiding this comment.
I think this should be added to a configuration, and perhaps disabled by default to avoid breaking changes.
Then a user can opt-in for both the recaptcha and the rate limiter.
pages/login/LoginHandler.inc.php
Outdated
| $session = $sessionManager->getUserSession(); | ||
|
|
||
| // Store reset requests with timestamps in the session | ||
| $resetRequests = $session->getSessionVar('passwordResetRequests') ?: []; |
There was a problem hiding this comment.
Hmm, if we store the retries on the session, then I think it's not very useful, as clearing the session cookie will be enough to skip the check.
I think this piece of code (rate limiter) should be removed or moved to another issue, there's a relevant discussion here: #11084
There was a problem hiding this comment.
Gotcha. I'll remove this piece of code for now.
| {if $captchaEnabled} | ||
| <div class="captcha"> | ||
| <div class="pkp_form_locale_field"> | ||
| {$reCaptchaHtml} | ||
| </div> | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
Just to follow the pattern of other places:
| {if $captchaEnabled} | |
| <div class="captcha"> | |
| <div class="pkp_form_locale_field"> | |
| {$reCaptchaHtml} | |
| </div> | |
| </div> | |
| {/if} | |
| {if $reCaptchaHtml} | |
| <div class="captcha"> | |
| <div class="pkp_form_locale_field"> | |
| {$reCaptchaHtml} | |
| </div> | |
| </div> | |
| {/if} |
pages/login/LoginHandler.inc.php
Outdated
|
|
||
| if (!$captchaValidator->isValid()) { | ||
| $templateMgr->assign([ | ||
| 'captchaEnabled' => true, |
There was a problem hiding this comment.
| 'captchaEnabled' => true, |
pages/login/LoginHandler.inc.php
Outdated
| } | ||
|
|
||
| // Check if this email has a recent request (within the last 10 minutes) | ||
| if (isset($resetRequests[$email]) && ($currentTime - $resetRequests[$email] < 600)) { |
There was a problem hiding this comment.
If the user was blocked, I think it's good to inform that a new request can be made in "X minutes"
pages/login/LoginHandler.inc.php
Outdated
|
|
||
| // Remove entries older than 1 hour (3600 seconds) | ||
| foreach ($resetRequests as $resetEmail => $timestamp) { | ||
| if ($currentTime - $timestamp > 3600) { |
There was a problem hiding this comment.
If the user is allowed to try again after 10 minutes, then I think we can clear the entries also after 10 minutes.
There was a problem hiding this comment.
@asmecher I'm not sure if the rate limiter should be added as part of this issue, but if we continue, perhaps it should be behind a configuration (could be even enabled by default, given the attacks which are happening out there), and we can add more settings (e.g. freezing time).
We should also include the reCaptcha on the login form (a feature that was added just to 3.4: #6539).
|
Hello @jonasraoni, just a heads-up that I implemented your change requests, as far as I also removed the rate limiter as you previously mentioned. Please let me know if I have something else to do in this issue and I'll update it. 😄 P.S.: I also implemented the Google ReCAPTCHA on the login page, as you mentioned in your comment above for the issue #6539. |
Description: Add ReCAPTCHA on the
userLostPassword.tplpage. Solves the issue #6984.