[1.0.x] SharePoint Connector

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 will not be changed by this application; 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 branch to review and the project name.

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, even to leave a comment similar to the following one. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• It is also preferable to wait before using a CLI tool → to report what needs to be changed, especially because the comment left from Code Review Administrators suggests to use PHP_CodeSniffer. Before that, manual reviews should be done.
• Reviewers should not copy-paste the output of a CLI tool. They should use a CLI tool only once per application. When they do that, they should later verify the code has been correctly changed; this means, for example, that adding a documentation comment that is not correct just to avoid to get a warning/error is not a correct change that should be reported in a further comment.
• 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 → .

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

sharepoint_connector.info.yml

core_version_requirement: ^8 || ^9 || ^10

Drupal releases before Drupal 8.7.7 do not recognize the core_version_requirement property. ^8 needs to be replaced with ^8.8.

/src/Controller/SharepointController.php

      if ($is_logging) {
        \Drupal::logger('sharepoint_connector')->error("An error occurred while posting to SharePoint: @error", ['@error' => $e->getMessage()]);
      }

Dependencies must be injected with the DI container.
To log exceptions, there is the watchdog_exception() function, or the OO code that function uses.

src/Controller/SharepointControllerTabs.php

  /**
   * Form builder object via Dependency Injection.
   *
   * @var \Drupal\Core\Form\FormBuilderInterface
   */
  protected $formBuilder;

That property is already defined from the parent class. There is no need to redefine it.

  /**
   * {@inheritdoc}
   */
  public function __construct(FormBuilderInterface $form_builder) {
    $this->formBuilder = $form_builder;
  }

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.

  public function page1() {
    return [
      'form' => $this->formBuilder->getForm('Drupal\sharepoint_connector\Form\SharepointConnectorForm'),
    ];
  }

There is no need to use that code, when the method of a form class can be directly used in a route definition.

/src/Form/SharepointConnectorForm.php

  /**
   * Config Factory object via Dependency Injection.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected $config;

That property is not used from the class.

$sharepoint_config = $this->configFactory->get('sharepoint_connector.settings');

Classes that extend ConfigFormBase use $this->config() to get a configuration object.

/src/SharepointConnector.php

  /**
   * GuzzleHTTP Client.
   *
   * @var string
   */
  public $client = '';

Either it is a GuzzleHttp\Client object or a string.

That class should rather be used for a service.

/sharepoint_connector.module

      if ($is_logging) {
        \Drupal::logger('sharepoint_connector')->info("SUCCESS! Created new entry in sharepoint with the following info: <pre>" . print_r($send_array, TRUE) . "</pre>");
      }

The first argument for info() must be a literal string, not a string concatenation.

Thank you for the feedback. These items have all been updated in the 1.0.x-dev branch.

Hi @RobbyMo

I love this module a lot and bit thx for your contribution to the Drupal world! Here are some comments I will leave related to your solution.

• It's recommended to add the hook _help in the .module file as it's missing
• I will also recommend to move these functions _sharepoint_connector_process_node_fields, _sharepoint_connector_get_field_value, _sharepoint_connector_get_disabled_api into the service somewhere. If there is logic from the SharePoint connector, it's better to keep it within the service
• I see this use Drupal\webform\WebformSubmissionInterface in the .module file which means, you use the webform module as a dependency. Please, add this dependency in the .info.yml file
• Missing the composer.jsonfile. You need it in order to set a requirement for the PHP version and also the required dependencies, for instance, to require drupal/webform
• Once you have the form with the config sharepoint_connector.settings, you need to add the default config into the config/install/*. Also, add the schema definition too. Same for the others sharepoint_connector_settings_types_form, sharepoint_connector_settings_form_fields, sharepoint_connector_settings_form_webform_fields, sharepoint_connector_settings_webforms_form
• It's good that you have this service SharepointConnector and you can also add those mentioned functions from the .mnodule file here. As a recommendation please, add the interface definition for it.

Thank you for the feedback. These items have all been addressed in the 1.0.x-dev branch.

src/Form/SharepointConnectorForm.php

    $type_default = (isset($connection_defaults[$this->getFormId() . '_connection_type']) && !empty($connection_defaults[$this->getFormId() . '_connection_type'])) ? $connection_defaults[$this->getFormId() . '_connection_type'] : '';
    $form['connection_fieldset'][$this->getFormId() . '_connection_type'] = [
      '#type' => 'select',
      '#title' => $this->t('Connection Type'),
      '#options' => [
        'client_tenant_id' => $this->t('Client/Tenant ID'),
      ],
      '#description' => $this->t('Select the type of connection you would like to use to connect to Sharepoint.'),
      '#default_value' => $type_default,
    ];

    $disabled_default = (isset($connection_defaults[$this->getFormId() . '_disable_api']) && !empty($connection_defaults[$this->getFormId() . '_disable_api'])) ? $connection_defaults[$this->getFormId() . '_disable_api'] : '';
    $form['connection_fieldset'][$this->getFormId() . '_disable_api'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Disable all calls to Sharepoint'),
      '#description' => $this->t('Killswitch - disable Sharepoint API connections entirely.'),
      '#default_value' => $disabled_default,
      '#prefix' => '<span style="line-height: 5px;">&nbsp;</span>',
    ];

    $log_default = (isset($connection_defaults[$this->getFormId() . '_log_requests']) && !empty($connection_defaults[$this->getFormId() . '_log_requests'])) ? $connection_defaults[$this->getFormId() . '_log_requests'] : '';
    $form['connection_fieldset'][$this->getFormId() . '_log_requests'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Log requests'),
      '#description' => $this->t('Log all Sharepoint requests and responses.'),
      '#default_value' => $log_default,
    ];

Why is the form ID added to each form element name? Inside the form, those form element names are already unique.

    $this->config('sharepoint_connector.settings')
      ->set('connection_fieldset', $form_state->getValue('connection_fieldset'))
      ->set('keys_fieldset', $form_state->getValue('keys_fieldset'))
      ->set('host_fieldset', $form_state->getValue('host_fieldset'))
      ->save();

$form_state->getValue() does not return values for fieldsets. (Fieldsets are render elements, not form elements. They cannot be used to submit values.)
You need to call $form_state->getValue() for each of the form elements.

      $form['keys_fieldset'][$this->getFormId() . '_client_id']['#default_value'] = '';
      $form['keys_fieldset'][$this->getFormId() . '_client_id']['#disabled'] = TRUE;
      $form['keys_fieldset'][$this->getFormId() . '_client_id']['#description'] = $form['keys_fieldset'][$this->getFormId() . '_client_id']['#description'] . ' (Overriden by settings.php)';

Form element descriptions must be translatable strings. That is valid also for strings added to existing descriptions.

src/Form/SharepointConnectorFormContentTypes.php

SharepointConnectorFormContentTypes does not have any submitForm() method, which means no value will be saved. If the purpose of the class is only showing values or links, it should not extend ConfigFormBase.

src/Access/WebformModuleCheck.php

  /**
   * Checks access to the Webforms tab.
   *
   * @param \Symfony\Component\Routing\Route $route
   *   The route to check against.
   *
   * @return \Drupal\Core\Access\AccessResultAllowed|\Drupal\Core\Access\AccessResultForbidden
   *   The access result.
   */
  public function access(Route $route) {
    return ($this->moduleHandler->moduleExists('webform')) ? AccessResult::allowed() : AccessResult::forbidden();
  }

Methods defined in a parent class or in an interface just need {@inheritDoc} as documentation comment.

A few notes more:

• The short description for class constructors is Constructs a new [class name] object. not Constructor. Long descriptions are not used for constructors.
• There is no need to add via Dependency Injection to property descriptions, nor specify it contains an object. The request stack. is a sufficient description, for example.

Thank you for the feedback. These items have all been addressed in the 1.0.x-dev branch. I noticed phpcs flagged the constructor short descriptions for being longer than 80 characters due to the class namespaces but from https://www.drupal.org/docs/develop/standards/php/php-coding-standards → it mentions "Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 characters." so I assume that it's fine. Please let me know if there are any issues.

Yes, the limit of 80 characters is not strict on those cases, especially when the full (namespaced) class name is expected.

I see this as a security issue:

https://git.drupalcode.org/project/sharepoint_connector/-/blob/1.0.x/src...

  public function access(Route $route) {
    return ($this->moduleHandler->moduleExists('webform')) ? AccessResult::allowed() : AccessResult::forbidden();
  }

In my eyes we should compliment with a permission or provide a neutral result in order to allow other existing permissions to return a forbidden if the user should not access to the route. Also the webform module is a dependency in in the info.yml file, so it seems like this access check is always true.

Thank you for the feedback. This item has been removed in the 1.0.x-dev branch as it is no longer necessary as the webform module is now a dependency as you mentioned. I will be looking to make this optional again in the near future and will incorporate in your suggestions. For my education would you be able to elaborate on why this is considered a security issue?

Also do you know at what point the module is deemed RTBC? Is it time based (X days without someone finding an issue) or does someone specific needs to do a specific validation or is there different criteria? Just wondering since we're looking to add in additional functionality but per the submission guidelines until this gets approved I am the only one who can make code updates to the project. Thanks!

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries → ' post on Responsible maintainers
• Maintainership →
• Git version control system →
• Issue procedures and etiquette →
• Maintaining and responding to issues for a project →
• Release naming conventions → .

You can find more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thank you for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

I thank also the dedicated reviewers as well.

Automatically closed - issue fixed for 2 weeks with no activity.