Check if authenticated or anonymous users have privileged access

Good progress, but some aspects have been misunderstood.

1. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -74,6 +74,18 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +      if ($role->id() != 'anonymous' && !$role->hasPermission('administer account settings')) {
   +        $sensor_result->addStatusMessage($role->label() . ' has privileged access but not for registration');
   ...
   +      elseif (!$role->isAdmin()) {
   +        $sensor_result->addStatusMessage($role->label() . ' has privileged access including registration');
   

   These checks need to be completely refactored. See below.

2. +++ b/src/Tests/MonitoringCoreWebTest.php
   @@ -155,31 +156,47 @@ class MonitoringCoreWebTest extends MonitoringTestBase {
   +    $this->assertTrue(strpos($result->getMessage(), '1 privileged user(s)') !== FALSE);
   ...
   +    $this->assertTrue(strpos($result->getMessage(), '2 privileged user(s), 1 new user(s)') !== FALSE);
   ...
   +    $this->assertTrue(strpos($result->getMessage(), '2 privileged user(s)') !== FALSE);
   ...
   +    $this->assertTrue(strpos($result->getMessage(), '2 privileged user(s), 1 changed user(s)') !== FALSE);
   ...
   +    $this->assertTrue(strpos($result->getMessage(), '2 privileged user(s)') !== FALSE);
   

   Please assert the whole message with Equal. No partial checks.

3. +++ b/src/Tests/MonitoringCoreWebTest.php
   @@ -155,31 +156,47 @@ class MonitoringCoreWebTest extends MonitoringTestBase {
   +      'administer account settings',
   

   That's not the point.
   I was referring to the site setting at admin/config/people/accounts:
   Radio "Who can register accounts?"
   A value "visitors" for user_register means they can register... (and possibly have a super privileged user = authenticated)
   We really only want to check the roles authenticated and anonymous. Not all others. Because these permissions a user can gain on his own.
   - as anonymous by just accessing the site and having super power
   - as authenticated by self registering and having super power
   And the third case is just to avoid bad practice to close registration and assign super privileged roles to all (authenticated) users. Thus warn. The admin should create a dedicated role for these cases.

From discussion with Sascha, we need to decide if monitoring all (authenticated) users in case of privileged access assigned makes sense. For accidental assignment, this (loading all and managing them) will kill the system. If the main purpose of this sensor is to identify risky accidental assignments, then we failed to reach the goal.

We should
- apply a strict order of authenticated users
- limit them to 500 in loading
- make a separate count query for the numbers (which needs API changes, because counting the array size does not work for this case.)

As a result, both a small set of authenticated users are well monitorable, and we have a meaningful warning if it was accidental in environments with a huge user base.

Last accessed column shows a "0" if a user never logged in.

You add a title "Table shows new and changed users who have privileged access."
I would shorten that to "New and changed users with privileged access"

And then below add a second table:
"Users with privileged access"
And yeah, list them. :-)

1. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -34,6 +34,13 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +  protected $list_size =500;
   

   code style. Please also check the full diff for style again.

2. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -74,6 +81,30 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +    $role_ids = array_intersect($role_ids, ['authenticated', 'anonymous']);
   

   Do not overwrite this variable. Pick a new name.

3. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -74,6 +81,30 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +    $role_labels = [];
   ...
   +      $role_labels[$role->id()] = $role->label();
   +      $sensor_result->addStatusMessage($role->label() . ' has privileged access');
   

   The idea of creating the $role_labels array here is to add one addStatusMessage statement if the list is !empty() and a comma implode.
   I think the language works best with changing the sentence to
   "Privileged access for roles @roles" and @roles is "Authenticated user", "Anonymous user" or both "Authenticated user, Anonymous user"

4. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -74,6 +81,30 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +    if (!empty($role_labels['anonymous'])) {
   ...
   +    if (!empty($role_labels['authenticated']) && $user_register != USER_REGISTER_ADMINISTRATORS_ONLY) {
   

   These checks should stay on the original $rold_ids

5. +++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
   @@ -89,9 +120,23 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
   +      ->range(0, $this->list_size);
   

   Add a comment about why we limit here. (See our issue discussion above.)

6. +++ b/src/Tests/MonitoringCoreWebTest.php
   @@ -180,6 +181,48 @@ class MonitoringCoreWebTest extends MonitoringTestBase {
   +    $this->assertEqual($result->getMessage(), '3 privileged user(s), Anonymous user has privileged access, Authenticated user has privileged access');
   

   As a result of the dumb loop above the message is redundant.
   Monitoring messages should be well crafted and minimalistically right to the point.
   Merge the two sentences as proposed above.
   Also it is plural: anonymous users and authenticated users.

Oh sorry about the feedback you added:

Yeah i also first thought the double dates are redundant, but last access really matters somehow, and change does.

About the discussion with berdir and not listing the users...
Well yeah, if we don't fully agree we could move this discussion into a followup.
But in general, verbose output is pretty OK to be verbose and if the sensor is on WARNING or ERROR and there are no change records, i really want to see relevant verbose information. With aggregator sensors, we display the unaggregated result for investigation. Here, i really think we should list all users that are privileged to have an idea.
Fetching these users through other channels (sql, code) for debugging is complex. We should create awareness.

Now that i think about it, we should even have a separate verbose output item that lists the roles (comma separated) that are considered to be highly privileged.

+++ b/src/Plugin/monitoring/SensorPlugin/UserIntegritySensorPlugin.php
@@ -74,6 +81,33 @@ class UserIntegritySensorPlugin extends SensorPluginBase implements ExtendedInfo
+      $role_labels[$role->id()] = $role->label() . 's';

We always try to avoid technical character mangling of natural language.
Removed the "s" and committed. :-)

I don't get that error above. It passed green and still indicated some 8 fails?