Improve role/capability handling in usermanager

plugins/CoreHome/vue/dist/CoreHome.umd.js

@@ -1868,23 +1868,33 @@ var AjaxHelper_AjaxHelper = /*#__PURE__*/function () {
             $(_this3.loadingElement).hide();
           }
 
-          if (response && response.result === 'error' && !_this3.useRegularCallbackInCaseOfError) {
+          var results = _this3.postParams.method === 'API.getBulkRequest' && Array.isArray(response) ? response : [response];
+          var errors = results.filter(function (r) {
+            return r.result === 'error';
+          }).map(function (r) {
+            return r.message;
+          });
+
+          if (errors && errors.length && !_this3.useRegularCallbackInCaseOfError) {
+            var errorMessage = errors.filter(function (e) {
+              return e.length;
+            }).join('<br />');
             var placeAt = null;
             var type = 'toast';
 
-            if ($(_this3.errorElement).length && response.message) {
+            if ($(_this3.errorElement).length && errorMessage.length) {
               $(_this3.errorElement).show();
               placeAt = _this3.errorElement;
               type = null;
             }
 
             var isLoggedIn = !document.querySelector('#login_form');
 
-            if (response.message && isLoggedIn) {
+            if (errorMessage && isLoggedIn) {
               var UI = window['require']('piwik/UI'); // eslint-disable-line
 
               var notification = new UI.Notification();
-              notification.show(response.message, {
+              notification.show(errorMessage, {
                 placeat: placeAt,
                 context: 'error',
                 type: type,
@@ -2117,8 +2127,17 @@ var AjaxHelper_AjaxHelper = /*#__PURE__*/function () {
       return helper.send().then(function (result) {
         var data = result instanceof AjaxHelper ? result.requestHandle.responseJSON : result; // check for error if not using default notification behavior
 
-        if (data.result === 'error') {
-          throw new ApiResponseError(data.message);
+        var results = helper.postParams.method === 'API.getBulkRequest' && Array.isArray(data) ? data : [data];
+        var errors = results.filter(function (r) {
+          return r.result === 'error';
+        }).map(function (r) {
+          return r.message;
+        });
+
+        if (errors.length) {
+          throw new ApiResponseError(errors.filter(function (e) {
+            return e.length;
+          }).join('\n'));
         }
 
         return result;

plugins/CoreHome/vue/src/AjaxHelper/AjaxHelper.ts

@@ -241,8 +241,11 @@ export default class AjaxHelper<T = any> { // eslint-disable-line
       const data = result instanceof AjaxHelper ? result.requestHandle!.responseJSON : result;
 
       // check for error if not using default notification behavior
-      if ((data as ErrorResponse).result === 'error') {
-        throw new ApiResponseError((data as ErrorResponse).message);
+      const results = helper.postParams.method === 'API.getBulkRequest' && Array.isArray(data) ? data : [data];
+      const errors = results.filter((r) => r.result === 'error').map((r) => r.message as string);
+
+      if (errors.length) {
+        throw new ApiResponseError(errors.filter((e) => e.length).join('\n'));
       }
 
       return result as R;
@@ -597,20 +600,24 @@ export default class AjaxHelper<T = any> { // eslint-disable-line
           $(this.loadingElement).hide();
         }
 
-        if (response && response.result === 'error' && !this.useRegularCallbackInCaseOfError) {
+        const results = this.postParams.method === 'API.getBulkRequest' && Array.isArray(response) ? response : [response];
+        const errors = results.filter((r) => r.result === 'error').map((r) => r.message as string);
+
+        if (errors && errors.length && !this.useRegularCallbackInCaseOfError) {
+          const errorMessage = errors.filter((e) => e.length).join('<br />');
           let placeAt = null;
           let type: string|null = 'toast';
-          if ($(this.errorElement).length && response.message) {
+          if ($(this.errorElement).length && errorMessage.length) {
             $(this.errorElement).show();
             placeAt = this.errorElement;
             type = null;
           }
 
           const isLoggedIn = !document.querySelector('#login_form');
-          if (response.message && isLoggedIn) {
+          if (errorMessage && isLoggedIn) {
             const UI = window['require']('piwik/UI'); // eslint-disable-line
             const notification = new UI.Notification();
-            notification.show(response.message, {
+            notification.show(errorMessage, {
               placeat: placeAt,
               context: 'error',
               type,

plugins/UsersManager/API.php

@@ -1149,21 +1149,20 @@ public function addCapabilities($userLogin, $capabilities, $idSites)
 
         [$sitesIdWithRole, $sitesIdWithCapability] = $this->getRolesAndCapabilitiesForLogin($userLogin);
 
+        foreach ($idSites as $idSite) {
+            if (!array_key_exists($idSite, $sitesIdWithRole)) {
+                throw new Exception(Piwik::translate('UsersManager_ExceptionNoCapabilitiesWithoutRole', [$userLogin, $idSite]));
+            }
+        }
+
         foreach ($capabilities as $entry) {
             $cap = $this->capabilityProvider->getCapability($entry);
 
             foreach ($idSites as $idSite) {
-                $hasRole = array_key_exists($idSite, $sitesIdWithRole);
-                $hasCapabilityAlready = array_key_exists($idSite, $sitesIdWithCapability) && in_array(
-                    $entry,
-                    $sitesIdWithCapability[$idSite],
-                    true
-                );
+                $hasCapabilityAlready = array_key_exists($idSite, $sitesIdWithCapability)
+                    && in_array($entry, $sitesIdWithCapability[$idSite], true);
 
-                // so far we are adding the capability only to people that also have a role...
-                // to be defined how to handle this... eg we are not throwing an exception currently
-                // as it might be used as part of bulk action etc.
-                if ($hasRole && !$hasCapabilityAlready) {
+                if (!$hasCapabilityAlready) {
                     $theRole = $sitesIdWithRole[$idSite];
                     if ($cap->hasRoleCapability($theRole)) {
                         // todo this behaviour needs to be defined...

plugins/UsersManager/Controller.php

@@ -94,20 +94,20 @@ public function index()
         $view->defaultReportSiteName = $defaultReportSiteName;
         $view->currentUserRole = Piwik::hasUserSuperUserAccess() ? 'superuser' : 'admin';
         $view->accessLevels = [
-            ['key' => 'noaccess', 'value' => Piwik::translate('UsersManager_PrivNone')],
-            ['key' => 'view', 'value' => Piwik::translate('UsersManager_PrivView')],
-            ['key' => 'write', 'value' => Piwik::translate('UsersManager_PrivWrite')],
-            ['key' => 'admin', 'value' => Piwik::translate('UsersManager_PrivAdmin')],
-            ['key' => 'superuser', 'value' => Piwik::translate('Installation_SuperUser'), 'disabled' => true],
+            ['key' => 'noaccess', 'value' => Piwik::translate('UsersManager_PrivNone'), 'type' => 'role'],
+            ['key' => 'view', 'value' => Piwik::translate('UsersManager_PrivView'), 'type' => 'role'],
+            ['key' => 'write', 'value' => Piwik::translate('UsersManager_PrivWrite'), 'type' => 'role'],
+            ['key' => 'admin', 'value' => Piwik::translate('UsersManager_PrivAdmin'), 'type' => 'role'],
+            ['key' => 'superuser', 'value' => Piwik::translate('Installation_SuperUser'), 'type' => 'role', 'disabled' => true],
         ];
         $view->filterAccessLevels = [
-            ['key' => '', 'value' => ''], // show all
-            ['key' => 'noaccess', 'value' => Piwik::translate('UsersManager_PrivNone')],
-            ['key' => 'some', 'value' => Piwik::translate('UsersManager_AtLeastView')],
-            ['key' => 'view', 'value' => Piwik::translate('UsersManager_PrivView')],
-            ['key' => 'write', 'value' => Piwik::translate('UsersManager_PrivWrite')],
-            ['key' => 'admin', 'value' => Piwik::translate('UsersManager_PrivAdmin')],
-            ['key' => 'superuser', 'value' => Piwik::translate('Installation_SuperUser')],
+            ['key' => '', 'value' => '', 'type' => 'role'], // show all
+            ['key' => 'noaccess', 'value' => Piwik::translate('UsersManager_PrivNone'), 'type' => 'role'],
+            ['key' => 'some', 'value' => Piwik::translate('UsersManager_AtLeastView'), 'type' => 'role'],
+            ['key' => 'view', 'value' => Piwik::translate('UsersManager_PrivView'), 'type' => 'role'],
+            ['key' => 'write', 'value' => Piwik::translate('UsersManager_PrivWrite'), 'type' => 'role'],
+            ['key' => 'admin', 'value' => Piwik::translate('UsersManager_PrivAdmin'), 'type' => 'role'],
+            ['key' => 'superuser', 'value' => Piwik::translate('Installation_SuperUser'), 'type' => 'role'],
         ];
 
         $view->statusAccessLevels = [
@@ -120,7 +120,9 @@ public function index()
         $capabilities = Request::processRequest('UsersManager.getAvailableCapabilities', [], []);
         foreach ($capabilities as $capability) {
             $capabilityEntry = [
-                'key' => $capability['id'], 'value' => $capability['category'] . ': ' . $capability['name'],
+                'key' => $capability['id'],
+                'value' => $capability['category'] . ': ' . $capability['name'],
+                'type' => 'capability'
             ];
             $view->accessLevels[] = $capabilityEntry;
             $view->filterAccessLevels[] = $capabilityEntry;

plugins/UsersManager/Sql/UserTableFilter.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Matomo - free/libre analytics platform
  *
@@ -8,7 +9,6 @@
 
 namespace Piwik\Plugins\UsersManager\Sql;
 
-
 use Piwik\Common;
 use Piwik\Piwik;
 
@@ -52,8 +52,9 @@ public function __construct($filterByRole, $filterByRoleSite, $filterSearch, $fi
         }
 
         // can only filter by superuser if current user is a superuser
-        if ($this->filterByRole == 'superuser'
-          && !Piwik::hasUserSuperUserAccess()
+        if (
+            $this->filterByRole == 'superuser'
+            && !Piwik::hasUserSuperUserAccess()
         ) {
             $this->filterByRole = null;
         }
@@ -121,7 +122,6 @@ public function getWhere()
 
     private function getAccessSelectSqlCondition()
     {
-        $sql = '';
         $bind = [];
 
         switch ($this->filterByRole) {
@@ -135,11 +135,12 @@ private function getAccessSelectSqlCondition()
                 $sql = "u.superuser_access = 1";
                 break;
             default:
-                $sql = "a.access = ?";
+                $sql = "u.login IN (SELECT login from " . Common::prefixTable('access') . " WHERE access = ? AND (idsite IS NULL OR idsite = ?))";
                 $bind[] = $this->filterByRole;
+                $bind[] = $this->filterByRoleSite;
                 break;
         }
 
         return [$sql, $bind];
     }
-}
+}

plugins/UsersManager/lang/en.json

@@ -61,6 +61,7 @@
         "ExceptionYouMustGrantSuperUserAccessFirst": "There has to be at least one user with Super User access. Please grant Super User access to another user first.",
         "ExceptionUserHasViewAccessAlready": "This user has access to this website already.",
         "ExceptionNoValueForUsernameOrEmail": "Please enter a username or email address.",
+        "ExceptionNoCapabilitiesWithoutRole": "Unable to grant any capabilities to user %1$s for idSite %2$s. The user needs at least view access.",
         "ExcludeVisitsViaCookie": "Exclude your visits using a cookie",
         "ForAnonymousUsersReportDateToLoadByDefault": "For anonymous users, report date to load by default",
         "GiveUserAccess": "Give '%1$s' %2$s access for %3$s.",

plugins/UsersManager/tests/Integration/APITest.php

@@ -1140,13 +1140,14 @@ public function test_addCapabilities_DoesNotAddSameCapabilityTwice()
 
     public function test_addCapabilities_DoesNotAddCapabilityToUserWithNoRole()
     {
+        self::expectException(\Exception::class);
+        self::expectExceptionMessage('UsersManager_ExceptionNoCapabilitiesWithoutRole');
+
         $access = $this->model->getSitesAccessFromUser($this->login);
 
         $this->assertEquals([], $access);
 
-        $this->api->addCapabilities($this->login, [TestCap2::ID, TestCap3::ID], [1]);
-
-        $this->assertEquals([], $access);
+        $this->api->addCapabilities($this->login, array(TestCap2::ID, TestCap3::ID), array(1));
     }
 
     public function test_addCapabilities_DoesNotAddCapabilitiesWhichAreIncludedInRoleAlready()

plugins/UsersManager/tests/UI/UsersManager_spec.js

@@ -525,14 +525,15 @@ describe("UsersManager", function () {
         await page.goto(url);
         await (await page.jQuery('.resend')).click();
         await page.waitForTimeout(500); // animation
-        await page.waitForSelector('.resend-invite-confirm-modal', { visible: true });
-        expect(await page.screenshotSelector('.usersManager')).to.matchImage('resend_popup');
+        const elem = await page.waitForSelector('.resend-invite-confirm-modal', { visible: true });
+        expect(await elem.screenshot()).to.matchImage('resend_popup');
     });
 
     it('should show resend success message', async function() {
         await (await page.jQuery('.resend-invite-confirm-modal .modal-close:not(.modal-no):visible')).click();
         await page.waitForSelector('#notificationContainer .notification');
-        expect(await page.screenshotSelector('.usersManager')).to.matchImage('resend_success');
+        await page.waitForNetworkIdle();
+        expect(await page.screenshotSelector('.usersManager, #notificationContainer .notification')).to.matchImage('resend_success');
     });