New Sensor: User integrity

As discussed with Berdir,
Implemented new hook: monitoring_user_update to update user data on updating a user.
sensor created to monitor these changes in users. As for now, the sensor result is the count of all the changes made to user accounts(usename, email, roles).
Verbose improvements and discard stored changes after the approving are yet to be implemented.
providing this for a feedback.

1. +++ b/config/install/monitoring.sensor_config.user_integrity.yml
   @@ -0,0 +1,11 @@
   +  time_interval_value: 86400
   

   There is no time interval here.

2. +++ b/monitoring.module
   @@ -644,3 +645,35 @@ function monitoring_search_api_index_update(IndexInterface $index) {
   +  if($user->getUsername() != $user->original->getUsername()){
   +
   +    $changed_user['name']['old_value'] = $user->original->getUsername();
   +    $changed_user['name']['new_value'] = $user->getUsername();
   +  }
   +
   +  if($user->getEmail() != $user->original->getEmail()){
   +    $changed_user['email']['old_value'] = $user->original->getEmail();
   +    $changed_user['email']['new_value'] = $user->getEmail();
   +  }
   +
   +  if($user->getRoles() != $user->original->getRoles()){
   +    $changed_user['roles']['old_value'] = $user->original->getRoles();
   +    $changed_user['roles']['new_value'] = $user->getRoles();
   +
   +  }
   +
   +  if(!empty($user)){
   +    $changed_user['id'] = $user->id();
   +    $changed_user['time_stamp'] = $user->getChangedTime();
   +    Drupal::keyValue('monitoring.users')->set($user->id().'_'.$user->getChangedTime(),$changed_user);
   +  }
   

   You need to store this by field, each change needs to be set() separately, including field name.

Verbose output added and filter added to monitor only users who gained administer privileges.
user data reset is yet to be implemented ( and tests :-) )

Hm, we need to discuss this. This approach is flawed. I don't know how to do this in a useful way.

As discussed with Berdir
The hook approach will not work, instead query all users for the relevant roles.
- add message about the number of privileged users
- load the users
- load all from key value,
- if empty don't compare, just collect. loop over users get name,email and password(but hash it), store this information in key value each by user id
- if not empty loop over users and instead of collecting compare to the data we have from key value.
- if user is not in key value data add message about the user (user name is new), and set to warning.
- if user is in key value data, compare name email and password hash, if mismatch set sensor state to warning and add a message (changes in user).
- in verbose show list of new admin users and list of changes
- add method getRestrictedRoles() for this need to get all roles and get al permissions filter by restricted access the loop over those, return roles that has at least one of those permissions.

As discussed above.

Some feedback.

1. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +      foreach ($all_users as $user) {
   +        \Drupal::keyValue('monitoring.users')->set($user->id(), $user);
   

   We don't want to store the whole user object, just the data we care about. an array with username, email and password hash hash. have a helper method to get that from a user. Then store that or compare with the data from keyvalue. $all_user is not all users, it is something like $current_users.

2. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +        $sensor_result->setMessage('No changes in users');
   

   use addStatusMessage(), that allows you to combine multiple messages together.

   What we want to display is the current user count, something like "N privileged users". In all scenarios, so you can do that as first think, before even checking for the key value store.

3. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +      $new_users = array_diff(array_keys($all_users), array_keys($current_users));
   +      if (empty($new_users)) {
   +        $sensor_result->setStatus(SensorResultInterface::STATUS_OK);
   +        $sensor_result->setMessage('Refer to verbose message');
   +      }
   +      else {
   +        $sensor_result->setStatus(SensorResultInterface::STATUS_WARNING);
   +        $sensor_result->setMessage(count($new_users) . ' new user(s)');
   +      }
   

   you can use array_diff_key() for the first line.

   Here we want to add two more status messages. "N new users", "N changed users".

   To find changed users, you need to compare the data in keyvalue with the current users, like verbose, just don't show it. Just keep a counter for changed, if new or changed is > 0, set status to warning and add a corresponding message.

4. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +    // Counts the payments from payment_status created within the configured
   +    // time interval.
   +    $statement = db_select('user__roles', 'ur');
   +    $statement
   +      ->distinct()
   +      ->addField('ur', 'roles_target_id');
   +    return ($statement->execute()->fetchCol());
   

   wrong comment.

   roles are entities, like everything else. Role::loadMultiple().

5. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +   * @return array
   +   *   Restricted roles.
   

   This would be more useful if you return an array of role entities, eyed by role id, then you can use array_keys() for the user call and display the role list in the verbose output.

   Instead of documenting as array, use \Drupal\some\Object[] or string[] to document what data structure you are returning.

6. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +    $avaliable_roles = $this->getAvailableRoles();
   +    $restricted_roles = array();
   +    foreach ($avaliable_roles as $role) {
   +      $permissions = Role::load($role)->getPermissions();
   

   This should be simpler when getAvailableRoles() returns role objects already.

Some feedback.

1. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +      foreach ($all_users as $user) {
   +        \Drupal::keyValue('monitoring.users')->set($user->id(), $user);
   

   We don't want to store the whole user object, just the data we care about. an array with username, email and password hash hash. have a helper method to get that from a user. Then store that or compare with the data from keyvalue. $all_user is not all users, it is something like $current_users.

2. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +        $sensor_result->setMessage('No changes in users');
   

   use addStatusMessage(), that allows you to combine multiple messages together.

   What we want to display is the current user count, something like "N privileged users". In all scenarios, so you can do that as first think, before even checking for the key value store.

3. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +      $new_users = array_diff(array_keys($all_users), array_keys($current_users));
   +      if (empty($new_users)) {
   +        $sensor_result->setStatus(SensorResultInterface::STATUS_OK);
   +        $sensor_result->setMessage('Refer to verbose message');
   +      }
   +      else {
   +        $sensor_result->setStatus(SensorResultInterface::STATUS_WARNING);
   +        $sensor_result->setMessage(count($new_users) . ' new user(s)');
   +      }
   

   you can use array_diff_key() for the first line.

   Here we want to add two more status messages. "N new users", "N changed users".

   To find changed users, you need to compare the data in keyvalue with the current users, like verbose, just don't show it. Just keep a counter for changed, if new or changed is > 0, set status to warning and add a corresponding message.

4. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +    // Counts the payments from payment_status created within the configured
   +    // time interval.
   +    $statement = db_select('user__roles', 'ur');
   +    $statement
   +      ->distinct()
   +      ->addField('ur', 'roles_target_id');
   +    return ($statement->execute()->fetchCol());
   

   wrong comment.

   roles are entities, like everything else. Role::loadMultiple().

5. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +   * @return array
   +   *   Restricted roles.
   

   This would be more useful if you return an array of role entities, eyed by role id, then you can use array_keys() for the user call and display the role list in the verbose output.

   Instead of documenting as array, use \Drupal\some\Object[] or string[] to document what data structure you are returning.

6. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,231 @@
   +    $avaliable_roles = $this->getAvailableRoles();
   +    $restricted_roles = array();
   +    foreach ($avaliable_roles as $role) {
   +      $permissions = Role::load($role)->getPermissions();
   

   This should be simpler when getAvailableRoles() returns role objects already.

Changed made as Berdir suggested.

Bigger picture is looking better now, so here is a detailed review.

Also needs tests.

1. +++ b/config/install/monitoring.sensor_config.user_integrity.yml
   @@ -0,0 +1,9 @@
   +label: 'Changed users'
   +description: 'Moniter cahnges in users'
   

   I think a better label would be "Privileged user integrity" as label, and something like "Monitors name and e-mail changes of users with access to restricted permissions."

2. +++ b/config/install/monitoring.sensor_config.user_integrity.yml
   @@ -0,0 +1,9 @@
   +value_label: 'Changes'
   +value_type: 'number'
   

   The sensor does not set a value, so there should be no value label and value type should be none.

3. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   + * A custom database query is used here instead of entity manager for
   + * performance reasons.
   

   Not correct anymore.

4. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   + *   label = @Translation("User Integrity"),
   + *   description = @Translation("Monitors the change of users and their permissions."),
   

   same label and description as for the config entity I think.

5. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   + *   addable = TRUE
   

   No.

6. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function resultVerbose(SensorResultInterface $result) {
   

   resultVerbose() should be further down in the class, we should start with the most important parts, runSensor() and the methods called there. Then this will be easier to understand if you read the other parts already.

7. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   +    $current_user = $this->loadCurrentUsers($role_ids);
   +    $current_user = $this->processUsers($current_user);
   

   why singular? also, see below on this.

8. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   +    // Create rows for old users.
   +    foreach ($old_user_ids as $id) {
   +      $changes = $this->getUserChanges($current_user[$id], $old_users[$id]);
   +      foreach ($changes as $key => $value) {
   +        $time_stamp = $current_user[$id]['changed'];
   

   You need to special case password here, then.

9. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -0,0 +1,278 @@
   +    $current_users = $this->loadCurrentUsers($role_ids);
   +    $current_users = $this->processUsers($current_users);
   

   I suggest you directly pass the return of loadCurrentUsers() to process users then here, instead of re-using the same variable for two very different things.

10. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +    $old_users = \Drupal::keyValue('monitoring.users')->getAll();
    

    Maybe $expected_users would be a better name here.

11. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +        $sensor_result->setStatus(SensorResultInterface::STATUS_OK);
    

    Should be set outside of the loop. Also, OK is the default, you should not have to set it, only set it to something else when you have to.

12. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +      $sensor_result->setMessage(count($current_users) . ' privileged users');
    ...
    +        $sensor_result->addStatusMessage(count($current_users) . ' privileged users');
    ...
    +        $sensor_result->addStatusMessage(count($new_users) . ' new user(s)');
    

    Move the first message up, do it directly after loading them, as status message.

    New users is not an if/else, it should be down additionally.

    The expected message if you have new and changed users should look like this: "17 privileged users, 3 new, 2 changed."

13. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +        $sensor_result->addStatusMessage($count . ' Changed user(s)');
    

    Changed lowercase.

14. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +        $sensor_result->setStatus(SensorResultInterface::STATUS_CRITICAL);
    

    Warning for this is also ok. It is something that you should review and then confirm, not something we know to be broken.

15. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    +
    +    $form['reset_users'] = array(
    +      '#type' => 'submit',
    +      '#value' => 'Reset user data',
    +      '#submit' => array(array($this, 'resetCurrentUsers')),
    +    );
    +    return $form;
    +  }
    ...
    +  /**
    +   * Resets current keyValue storage.
    +   *
    +   * @return array
    +   *   Available users.
    +   */
    +  public function resetCurrentUsers(array $form, FormStateInterface $form_state) {
    +    \Drupal::keyValue('monitoring.users')->deleteAll();
    +    return $form;
    +  }
    

    #value needs to be translated, and I think it should say something like "Confirm privileged user changes". We should probably also only show it if there is something to confirm. More and more reasons to show it on verbose, not here... keep for now.

    Also, move form related methods together, after other helper functions and make it clear that the second is a submit callback, by naming it e.g. submitConfirmPrivilegedUsers()

16. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +    $uids = \Drupal::entityQuery('user')
    ...
    +    /** @var \Drupal\user\PermissionHandlerInterface $permission_handler */
    +    $permission_handler = \Drupal::service('user.permissions');
    

    Let's inject those services through create()

17. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +    $users = User::loadMultiple($uids);
    +    return $users;
    

    No need for $users, return immediately, same for roles above.

18. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +   * @param string[] $new_values, $old_values
    +   *   User data to compare.
    

    This is not how @param works ;)

    two params, and they are not string[]. Also document the format you expect, refer to processUsers(), which builds it for you.

19. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +  protected function getUserChanges(array $new_values, array $old_values) {
    

    Here as well, current/expected values is IMHO better than new/old.

20. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +    if ($new_values['password'] != $old_values['password']) {
    +      $changes['password']['old_value'] = $old_values['password'];
    +      $changes['password']['new_value'] = $new_values['password'];
    +    }
    

    The old and new value is not relevant, we just know that it changed, just set $changes['password'] = TRUE or so.

21. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +      foreach ($permissions as $permission) {
    +        if (in_array($permission, $restricted_permissions)) {
    +          $restricted_roles[] = $role->id();
    +          break;
    +        }
    

    You can save one loop by using http://php.net/array_intersect. If there is an intersect between restricted permissions and role permissions, then the role is restricted.

22. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +  /**
    +   * Process user changes.
    +   *
    +   * @param \Drupal\user\Entity\User[] $users
    +   *  Roles to filter users.
    +   *
    +   * @return string[]
    +   *   Processed user data.
    +   */
    +  protected function processUsers(array $users) {
    +    $processed_users = array();
    

    method description is very unclear, what is "process". I suggest somehting like "Converts user entities to raw values". Possibly rename the method accordinly, convertToUserValues() or so?

    Description of the @param is also not correct, has nothing to do with roles.

    And @return is also not a string[], just array here, and document something like "List of arrays with keys a, b, c, d."

23. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
    @@ -0,0 +1,278 @@
    +      $processed_users[$id]['password'] = md5($user->getPassword());
    

    Not sure if we should use md5() here, let's use the the hash() method from the password service.

The changed made according to Berdir's long review.
Attached a screenshot of sensor result and verbose message.

Ok, interdiff looks ok, just the submit button value was not updated as far as I could see.

I think it would make sense to make the same change to old/new in the array keys and the output, and display the current value first. That way, it doesn't matter if "expected value" is empty, e.g. for new users and password changes.

Also, as indirectly mentioned above, what would also be useful to show would be the list of roles we are looking for, just a simple comma separated list, "Roles with restricted permissions: Role 1, Role 2, Role 3". You can use #type item, with #label and #markup.

I don't think the separate new user column is helpful, we can see that in current value. Instead, let's display #theme => 'username', so that we have the current username and a link to his profile. you need to change the table render array to just add the rows with + $rows, instead of #rows, then the elements can be render arrays.

And i think more and more that the reset button should be there, and not in the edit form. Try to just add it below the table, it should just work, for now.

Verbose message improved, a link to user profile added, user id, new user columns dropped from the table
array keys corrected to current/expected
reset button is now on the sensor details page (in verbose message).

Schema corrected.

Reset button moved back to sensor edit page,
Core tests and UI tests added.

2 . $changed_users_ids contain the ids of the users changed, it's not the changed ids, I agree it looks confusing a bit but couldn't come up with a better name ;)
any suggestions? :)
patch rerolled

Committed and pushed.