[VYMCA-64] CSV Security Enhancement #68
Conversation
| @@ -1,2 +1,7 @@ | |||
| enable_recaptcha: 1 | |||
| enable_recaptcha: 0 | |||
There was a problem hiding this comment.
are we going to disable captcha for current sites via hood_update_N?
There was a problem hiding this comment.
In this case ReCaptcha disabled for builds, without sitekey it not works
There was a problem hiding this comment.
So users would need to enable it manually after installation, right?
I guess instead of changing defaults to 'no captcha' it's better to tune build system to disable captcha via config set . Defaults should be production ready, not build ready
There was a problem hiding this comment.
I agree, reverted this to the previous state. By default on builds we have dummy plugin, so I think no need to tune the build system now.
modules/openy_gc_auth/modules/openy_gc_auth_custom/openy_gc_auth_custom.info.yml
Show resolved
Hide resolved
|
|
||
| // Set proper permissions to access the new endpoint. | ||
| openy_gc_auth_custom_install(); |
There was a problem hiding this comment.
let's avoid of using hook_install
Imaging we'd be changing hook install in the future and somebody does upgrade of his site by running this hook_update_N
This could cause unexpected upgrade path issues
Let's just use some custom function or duplicate a code, which is fine in hook_update_N
| @@ -24,6 +24,11 @@ public function defaultConfiguration():array { | |||
| return [ | |||
| 'enable_recaptcha' => TRUE, | |||
| 'api_endpoint' => '/openy-gc-auth/provider/custom/login', | |||
| 'enable_email_verification' => TRUE, | |||
| 'email_verification_api_endpoint' => '/openy-gc-auth/provider/custom/login-by-link', | |||
| 'email_verification_link_life_time' => 14400, | |||
There was a problem hiding this comment.
class const? - a must
config value with a form? - an option for future
| $this->configuration['enable_email_verification'] = $form_state->getValue('enable_email_verification'); | ||
| $this->configuration['email_verification_api_endpoint'] = $form_state->getValue('email_verification_api_endpoint'); | ||
| $this->configuration['email_verification_link_life_time'] = $form_state->getValue('email_verification_link_life_time'); | ||
| // TODO: check that text_format works on submit. |
There was a problem hiding this comment.
checked?
Is Paige aware and is it in project backlog?
| if (!$resp->isSuccess()) { | ||
| return $this->errorResponse($this->t('ReCaptcha token invalid.'), 400); | ||
| $validation_result = $this->validateRecaptcha($content, $request); | ||
| if ($validation_result instanceof ModifiedResourceResponse) { |
There was a problem hiding this comment.
what happens if not?
Any error handling in UI?
There was a problem hiding this comment.
validation_result is instanceof ModifiedResourceResponse only in case of error, so we return error response here.
| if ($provider_config->get('enable_email_verification')) { | ||
| if (!$user->isActive()) { | ||
| $verification_result = $this->sendEmailVerification($user, $content, $request, $provider_config); | ||
| if ($verification_result instanceof ModifiedResourceResponse) { |
|
|
||
| $config = $this->configFactory->get('recaptcha.settings'); | ||
| $recaptcha_secret_key = $config->get('secret_key'); | ||
| $recaptcha = new ReCaptcha($recaptcha_secret_key, new Drupal8Post()); |
There was a problem hiding this comment.
there no any services in reCaptcha module, this code was reused from recaptcha_captcha_validation
/**
* CAPTCHA Callback; Validates the reCAPTCHA code.
*/
function recaptcha_captcha_validation($solution, $response, $element, $form_state) {
$config = \Drupal::config('recaptcha.settings');
$recaptcha_secret_key = $config->get('secret_key');
if (empty($_POST['g-recaptcha-response']) || empty($recaptcha_secret_key)) {
return FALSE;
}
$recaptcha = new ReCaptcha($recaptcha_secret_key, new Drupal8Post());
if ($config->get('verify_hostname')) {
$recaptcha->setExpectedHostname($_SERVER['SERVER_NAME']);
}Maybe we can improve this later, I would prefer to leave it as is if it's not very urgent.
|
Code is ok. |
User story:
As a Virtual Y site owner, I want users to validate their email address before accessing Virtual Y, so that I can ensure only my real members are visiting my module/website.
Steps fo review:
Created test users emails: