[D8] Real Time User Behavior

Thank you for applying! rtub-dev.8.x isn't a correct branch name, which eventually would be 8.x-1.x. Wrong branches need to be removed.

PAEeview say readme missing. Please add it

According to Drupal standards, modules should include a useful README.file.
https://www.drupal.org/docs/develop/documenting-your-project/module-docu...

Please create a dev release to enable automated testing.

Thank you for the contribution!

Please, fix the Pareview mentioned issue (at first):
README.md or README.txt is missing, see the guidelines for in-project documentation.

Then the project will go through the manual security review process.
Please, be patience through the whole review process.

Thank you for the contribution!

1. Please, update the deprecated constant REQUEST_TIME into your code:
The REQUEST_TIME global constant has also been deprecated, scheduled to be removed before 9.0.0
See more: https://www.drupal.org/node/2785211
Replace the REQUEST_TIME with the \Drupal::time()->getRequestTime()

2. Replace the deprecated UrlGeneratorTrait:
trait \Drupal\Core\Routing\UrlGeneratorTrait is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Url instead.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2...

3. EntityManagerInterface is deprecated in the latest Drupal 8 version and should be replaced with EntityTypeManagerInterface.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

I have not found any security related issues into the code.

Thanks for you contribution!

1. PostNodeDataController: this is vulnerable to access bypass attacks. An attacker can send an arbitrary user ID, so they can write arbitrary events for other users. You need to take the user ID from the current logged in user and not from the sent POST data.
2. PostNodeDataController: a second vulnerability is the event type. An attacker can send arbitrary event types and messages, so they could send fake logout events for example. You could generate a HMAC for the JS settings you are creating with Crypt::hmacBase64(). The data would be a string concantenated of all the setting values. The key would be Settings::getHashSalt(). If you exclude the user ID then it would even work for cached node pages since the HMAC would be the same for all users.
3. PostNodeDataController: a potential third vulnerability is CSRF. An attacker can trick users into POST requests so that an event record is written. We prevent that usually with the Form API or by validating a CSRF token. However - in the case of tracking views/clicks CSRF is most of the time not a useful attack vector, so for example Drupal core's statistics module also does not prevent against CSRF. So I consider this fine.

Please fix vulnerability 1 and 2, then we can continue here.

Thanks for your contribution.
Review of the 8.x-1.x branch (commit dfcf442):
No automated test cases were found, did you consider writing PHPUnit tests? This is not a requirement but encouraged for professional software development.

Error in this line , add a space

$newStr = $event['eData'] . " | " . $event['eType'];

https://git.drupalcode.org/project/rtub/blob/8.x-1.x/src/Controller/Post...

Best practical used function.
#Example

/**
 * Returns data for "hmacBase64".
 *
 * @param $title
 *  Node title.
 * @param $type
 *  Event type.
 * @return string
 */
function _get_cript_data($title, $type) {
  return "User on {$title} | {$type}";
}

And use

$eStr = Crypt::hmacBase64(_get_cript_data($node->getTitle(), 'nodeview'), Settings::getHashSalt());

Please view csrfToken and system route system.csrftoken for get user token.

UP:
Why use js for check node view?

@maheshkp92 Good question.

I update the summary only.

Thanks for the contribution @maheshkp92 !

I have not find any security related issues in the code.

Thanks

    $newStr = "User on {$title} | {$type}";
    $eStr = Crypt::hmacBase64($newStr, Settings::getHashSalt());

    if ($event['eToken'] == $eStr) {
      // Valid post request.
      $connection = $this->connection;
      $connection->insert('rtub')
        ->fields([
          'uid' => $this->currentUser()->id(),
          'event_type' => $type,
          'event_timestamp' => $this->time->getRequestTime(),
          'event_data' => "User on {$title} Page",
        ])->execute();

      $response['status'] = TRUE;
      return new JsonResponse($response);
    }

Hashes are not checked using the == operator. There is a PHP function to compare two hashes, which is used also from Drupal.

function rtub_preprocess_node(&$variables) {
  $node = $variables['node'];
  $ntype = $node->getType();
  $user = \Drupal::currentUser();
  // Do not track data for Anonymous users.
  if ($user->isAnonymous()) {
    return;
  }
  // Check User permission.
  if (!$user->hasPermission("track $ntype view content")) {
    return;
  }

  if (!(\Drupal::service('path.matcher')->isFrontPage())) {
    // Do not track front page.
    $variables['#attached']['library'][] = 'rtub/rtub-js';
    $variables['#attached']['drupalSettings']['rtub']['nodeview']['eventData'] = $node->getTitle();
    $variables['#attached']['drupalSettings']['rtub']['nodeview']['eventType'] = "nodeview";

    $eStr = Crypt::hmacBase64(_get_cript_data($node->getTitle(), 'nodeview'), Settings::getHashSalt());

    $variables['#attached']['drupalSettings']['rtub']['nodeview']['eventToken'] = $eStr;
  }
}

Since the code isn't executed for the anonymous users, it probably makes more sense to use the csrf_token service.

  if (!$current_user->isAnonymous()) {
    // Track activity for login users.
    try {
      $connection = \Drupal::database();
      $connection->insert('rtub')
        ->fields([
          'uid' => $uid,
          'event_type' => $eventName,
          'event_timestamp' => $event_time,
          'event_data' => $message,
        ])->execute();

    }
    catch (exception $e) {
      \Drupal::logger('rtub')->warning(print_r('Something went wrong', TRUE));
    }
  }

There isn't no need to use print_r() to pass a string to warning(). Something went wrong is also a message that doesn't allow to understand what happened; it should be replaced from the message associated with the exception.

function _get_cript_data($title, $type) {
  return "User on {$title} | {$type}";
}

Every function should be prefixed from the module machine name. A comment on this application already suggested to remove it and use the string directly.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, 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 all the dedicated reviewers as well.