[1.0.x] Abstract API Email Validator

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

1. FILE: abstract_email_validation.module

/**
 * @file
 * Primary module hooks for email validation using abstract api module.
 */

Drupal does not have primary and secondary hooks. Instead of that, it is preferable to use the usual description: Hook implementations for the [module name] module. where [module name] is the name of the module given in its .info.yml file.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function abstract_email_validation_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state) {

The description for that hook should also say for which form that hook is implemented, either by indicating that with the name of the class that implements the form (namespace included) or the form ID (which is usually indicated by getFormId()).

2. FILE: src/Validate/EmailValidator.php

  /**
   * UserSettings constructor.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory.
   * @param \Drupal\Core\Logger\LoggerChannelFactoryInterface $logger
   *   The logger channel factory.
   * @param \Drupal\abstract_email_validation\Http\AbstractApi $abstract_api_handler
   *   Abstract Api Handler.
   */
  public function __construct(

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.

Hi @vishal.kadam, I've changed the codebase as per your suggestion in #4 with the following commit.

Thanks !

Rest seems fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

It is a very well-written module. Looks pretty good already. I also appreciate that you have been using GitlabCI directly from the beginning. Here are some notes that I collected while looking through the code:

1. Why are you limiting the allowed entity types in abstract_email_validation_entity_bundle_field_info_alter()? Users may be confused about whether email validation works on the entity field if the entity type is not on the list.
2. In abstract_email_validation_custom_form_validate() you probably misnamed the $abstract_api_service variable. Should be renamed to, e.g., $email_validator.
3. The constructor comment in EmailValidator exceeds 80 characters.
4. There is a pattern throughout your code where you assume that array keys exist. For example, in EmailValidator::getQualityScoreStatus() you assume that the $response has the array key quality_score. However, this response comes from an external source, and the content may change. Then your code would cause a WSOD. This is not only a bad pattern for external content. You can make the same argument from a "method perspective". How can a method trust another method to deliver an array in the correct shape? (Answer: with static analysis, but you are not doing that.)
5. In EmailValidator::validate() you can get the $detailed_settings later when they are needed.
6. I would rename the EmailValidator::detailedValidation() method to passesDetailedValidation(). Also, the $response variable in that method is confusing - maybe use $pass.
7. I would rename the EmailValidator::checkBlacklistEmail() method to isBlacklistedEmail().
8. I would rename the EmailValidator::checkBlacklistDomain() method to isBlacklistedDomain().
9. All your methods in EmailValidator are public, but the only service method is validate(). It is good practice to define the interface of your service and only expose the necessary methods.
10. I would rename the AbstractApi service to AbstractApiClient.
11. Following the previous note, you are naming the injected service AbstractApiHandler in the EmailValidator. But this is not a handler. Use $abstractApiClient.
12. The service classes for the email validator and Abstract API client are currently in the folders Validate and Http. This is rather unusual. Please place them in the Service folder.
13. Why is the AbstractEmailValidationSettingsForm class final?
14. For the error messages in the form validation in AbstractEmailValidationSettingsForm, please look up the difference between "i.e." and "e.g.", and how to place the commas.
15. In AbstractEmailValidationConstraintValidator the PHP docs for the $emailValidatorService property is wrong. Also, just call it $emailValidator.
16. In AbstractEmailValidationConstraintValidator this might be a problem:
    

    $this->context->addViolation($this->t($custom_error_message, [
      '%email' => $field_value['value'],
    ]));
    

    I know the custom error message comes from the config form that only administrators can access. But this is against Drupal security practices.

17. I would rename AbstractApi::callAbstractApi() to, e.g., validateEmail().
18. You are inconsistently typing properties.
19. Please review your code and check comments for coding standards, especially "third person for function" comments. See, e.g., https://www.drupal.org/node/1354#functions → .
20. Consider using property promotion for your constructors. You may need to bump the core version requirements for this.
21. Consider adding return types to all methods.
22. Consider adding a PHPStan configuration file phpstan.neon for your GitlabCI. The module is already passing the static analysis on level 1. And you can easily get to level 5 with some minor tweaks.

Most of the comments here are related to code style and best practices. Only point 16 is relevant for security.

Thanks @simonbaese for the review.
For point 16 will you please guide us on what is expected or share some documentation that mentions what should be done in this case?

Just do not make the error message configurable. If a website administrator wants to change the message, they can do so directly through interface translation.

Values coming from a configuration object, as in this case, can be translated via the Configuration Translation module, as described in Translating configuration → . Keep in mind the note reported in that page.

Configuration translation relies on having a correct configuration schema → to provide translations. So, every module must provide a correct schema.

Every site that wants to translate configuration values needs to install the Configuration Translation module. The modules providing those configuration values just retrieve them from a configuration object as they usually do.

HI @simonbaese & avpaderno,
We have made the changes as per your comments. Please review the MR.
MR

@indranil roy The reported changes need to be committed in the project repository. We do not review merge requests.

Actually, the merge request has been merged.

First of all, good job on following through with all of the recommendations. Your work elevates the code quality drastically. Just a quick scan - not a full review.

1. You are using property promotion incorrectly. See, e.g., PHP 8: Constructor property promotion for reference.
2. Code styling: Use {@inheritdoc} or {@inheritDoc} consistently. In Drupal Core {@inheritdoc} is used.
3. Code styling: Use capitalized acronyms in comments. For example, use "API" and "ID".
4. Code styling: Follow Drupal coding standards regarding method comments, i.e., "Use a third person verb to start the summary of a class, interface, or method." See API documentation and comment standards → for reference.
5. If the PHPStan baseline is empty, it can be omitted.

Hi @simonbaese, We have updated the codebase of the module as per your recommendations in #15. Please review the chnages.
Thanks!

This is an application from indranil roy. Only indranil roy can make commits while this application is open and only indranil roy can change this application status.

Sorry @avpaderno, We will keep this in mind next time.
Can you please review the changes?

src/Form/AbstractEmailValidationSettingsForm.php

    $form['details']['smtp'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Enable the email validation based on SMTP.'),
      '#description' => $this->t('Validate using the value of "is_smtp_valid" key of the API response.'),
      '#default_value' => $config->get('detailed_settings.smtp'),
    ];
    $form['details']['mx_found'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Enable the email validation based on MX_FOUND.'),
      '#description' => $this->t('Validate using the value of "mx_found" key of the API response.'),
      '#default_value' => $config->get('detailed_settings.mx_found'),
    ];
    $form['details']['quality_score'] = [
      '#type' => 'number',
      '#title' => $this->t('Enter the desired QUALITY_SCORE of email.'),
      '#description' => $this->t('Validate using the value of "quality_score" key of the API response. <br> It varies between 0.0 to 1.0. Recommended the "quality_score" is >= 0.8'),
      '#default_value' => $config->get('detailed_settings.quality_score'),
      '#min' => 0.1,
      '#max' => 1.0,
      '#step' => 0.1,
    ];
    $form['details']['deliverability'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Enable the email validation based on Deliverability.'),
      '#description' => $this->t('Validate using the value of "deliverability" key of the API response.'),
      '#default_value' => $config->get('detailed_settings.deliverability'),
    ];

Instead of using single checkbox form elements, the code should use a single checkboxes form element. With the latter, it is not even necessary to verify one or more checkboxes have been checked; it is sufficient to mark the form element required.

src/Validate/EmailValidator.php

/**
 * Provides the services to validate email ID.
 *
 * This class contains different functions for validation based on
 * Abstract API response and the different settings set by the admin user.
 */
class EmailValidator implements EmailValidatorInterface {

There is no need to say Provides the services which is not true since a class implements methods; even in the case service were used as "synonym" of methods, that could be said for every classes.

src/Http/AbstractApi.php

  /**
   * Request the Abstract API and send the response.
   *
   * @param string $email
   *   The email address to validate.
   *
   * @return array
   *   TRUE if the email is valid, FALSE otherwise.
   */
  public function validateEmail(string $email): array {
    $result = [];
    $config = $this->configFactory->getEditable('abstract_email_validation.settings');
    $api_base_url = $config->get('api_url');
    $api_key = $config->get('api_key');
    try {
      $response = $this->httpClient->request('GET', $api_base_url, [
        'query' => [
          'api_key' => $api_key,
          'email' => $email,
        ],
      ]);
      $result['status_code'] = $response->getStatusCode();
      $data = JSON::decode($response->getBody());
      $result['data'] = $data;
    }
    catch (\Exception $e) {
      $result['status_code'] = $e->getCode();
      $logger_channel = $this->loggerFactory->get('abstract_email_validation');
      $logger_channel->error('API Request failed: @message', ['@message' => $e->getMessage()]);
    }
    return $result;
  }

That is not the correct description for a method that validates an email address. The fact it uses an external service (which is not made clear by Request the Abstract API) is secondary.

src/Http/AbstractApi.php

/**
 * Service for requesting Abstract API.
 */
class AbstractApi {

That short description is merely repeating the class name. It does not make clear what type of requests the class handles, nor what Abstract API means in this context.

abstract_email_validation.module

function abstract_email_validation_help($route_name, RouteMatchInterface $route_match) {
  switch ($route_name) {
    case 'help.page.abstract_email_validation':
      $text = file_get_contents(__DIR__ . '/README.md');
      if (!\Drupal::moduleHandler()->moduleExists('markdown')) {
        return '<pre>' . Html::escape($text) . '</pre>';
      }
      else {
        // Use the Markdown filter to render the README.
        $filter_manager = \Drupal::service('plugin.manager.filter');
        $settings = \Drupal::configFactory()->get('markdown.settings')->getRawData();
        $config = ['settings' => $settings];
        $filter = $filter_manager->createInstance('markdown', $config);
        return $filter->process($text, 'en');
      }
  }
  return NULL;

Strings shown in the user interface must be translatable. The content of a file is not translatable. Furthermore, the content of a README file does not contain information useful, once the module is installed. Once the module is installed, people do not need to see how to install it, since they already installed it. Even knowing who the maintainers are is information useful once the module is installed.

A PHP function does not need to explicitly return NULL.

/**
 * Implements hook_form_FORM_ID_alter().
 *
 * Altering form ID field_config_edit_form.
 */
function abstract_email_validation_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state) {

The correct description for that hook is Implements hook_form_FORM_ID_alter() for [form ID]. where [form ID] is usually the fully-namespaced name of the form class.

  $custom_error_message = $config->get('custom_error_message');

  if (isset($email_address) && !empty($email_address)) {
    $result = $email_validator->validate($email_address);
    if (!$result['status']) {
      if (isset($result['message'])) {
        $form_state->setErrorByName($element['#name'], $result['message']);
      }
      else {
        // phpcs:ignore Drupal.Semantics.FunctionT.NotLiteralString
        $form_state->setErrorByName($element['#name'], t($custom_error_message, ['%email' => $email_address]));
      }
    }
  }

That is not how configuration values are translated. Translating configuration → explains how configuration values are translated.

Configuration translation relies on having a correct configuration schema → to provide translations. So, every module must provide a correct schema.

Furthermore, passing as first argument of t() arbitrary user input is considered a security issue.

    if (is_array($element)
      && !empty($element['#abstract_email_validation'])
      && $element['#type'] == 'email'
      ) {

Since modules should not throw warning, that code should first verify $element['#type'] has been set.
As per Drupal coding standards → , Conditions should not be wrapped into multiple lines.

I am changing priority as per Issue priorities → .