Manage dashboard access rights per Group and per User - #144
Manage dashboard access rights per Group and per User#144MichaelRodriguesOficial wants to merge 3 commits into
Conversation
| * | ||
| * @return bool | ||
| */ | ||
| public function showRightsForm($itemtype, $itemsId, $options = []) |
There was a problem hiding this comment.
You should use Twig templates instead.
Direct echo statements in PHP are no longer allowed. All output should be rendered through Twig templates to comply with the current coding standards.
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | ||
| { | ||
| foreach ($groupIds as $groupId) { | ||
| if (self::canItemViewDashboard(Group::class, $groupId, $dashboardUuid)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
canGroupsViewDashboard() makes one DB query per group (via getItemRightForDashboard). It is called from canCurrentUserViewDashboard(), which is itself called inside the array_filter loop in showForCentral() — once per dashboard. With D dashboards and G groups this produces D×G queries on every Central page load.
Replace with a single IN query on items_id — fetches all matching group rows at once and filters & READ in PHP. Apply the same pattern to canGroupsViewDashboards().
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | |
| { | |
| foreach ($groupIds as $groupId) { | |
| if (self::canItemViewDashboard(Group::class, $groupId, $dashboardUuid)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | |
| { | |
| if (empty($groupIds)) { | |
| return false; | |
| } | |
| /** @var DBmysql $DB */ | |
| global $DB; | |
| $iterator = $DB->request([ | |
| 'FROM' => self::getTable(), | |
| 'WHERE' => [ | |
| 'itemtype' => Group::class, | |
| 'items_id' => $groupIds, | |
| 'dashboard_uuid' => $dashboardUuid, | |
| ], | |
| ]); | |
| foreach ($iterator as $right) { | |
| if (($right['rights'] & READ) !== 0) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| `id` int {$default_key_sign} NOT NULL AUTO_INCREMENT, | ||
| `itemtype` varchar(100) NOT NULL, | ||
| `items_id` int {$default_key_sign} NOT NULL, | ||
| `dashboard_uuid` int NOT NULL, |
There was a problem hiding this comment.
should not be dashboard_id ?
| public static function canGroupsViewDashboards(array $groupIds): bool | ||
| { | ||
| foreach ($groupIds as $groupId) { | ||
| if (self::canItemViewDashboards(Group::class, $groupId)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Check if any group from the given list is able to view the given dashboard. | ||
| * | ||
| * @param int[] $groupIds | ||
| * @param integer $dashboardUuid | ||
| * | ||
| * @return boolean | ||
| */ | ||
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | ||
| { | ||
| foreach ($groupIds as $groupId) { | ||
| if (self::canItemViewDashboard(Group::class, $groupId, $dashboardUuid)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
canGroupsViewDashboard() (line 228) issues one DB query per group via getItemRightForDashboard(), and it's called once per dashboard from the array_filter loop in showForCentral() — D dashboards × G groups = D×G queries on every Central page load. stonebuzz's suggestion replaces it with a single IN-query on items_id, filtering & READ in PHP, and asks for the same pattern in canGroupsViewDashboards() (line 209), which has the identical per-group query loop.
Beyond the group path: canItemViewDashboard(User::class, ...) is also called once per dashboard from the same array_filter (via canCurrentUserViewDashboard()), adding D more single-row queries. Fixing only the group path still leaves D user queries per page load. Worth batching the User::class and Group::class checks in the same pass once at the top of showForCentral() rather than per-dashboard.
| public static function canGroupsViewDashboards(array $groupIds): bool | |
| { | |
| foreach ($groupIds as $groupId) { | |
| if (self::canItemViewDashboards(Group::class, $groupId)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| /** | |
| * Check if any group from the given list is able to view the given dashboard. | |
| * | |
| * @param int[] $groupIds | |
| * @param integer $dashboardUuid | |
| * | |
| * @return boolean | |
| */ | |
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | |
| { | |
| foreach ($groupIds as $groupId) { | |
| if (self::canItemViewDashboard(Group::class, $groupId, $dashboardUuid)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public static function canGroupsViewDashboards(array $groupIds): bool | |
| { | |
| if (empty($groupIds)) { | |
| return false; | |
| } | |
| /** @var DBmysql $DB */ | |
| global $DB; | |
| $iterator = $DB->request([ | |
| 'FROM' => self::getTable(), | |
| 'WHERE' => [ | |
| 'itemtype' => Group::class, | |
| 'items_id' => $groupIds, | |
| ], | |
| ]); | |
| foreach ($iterator as $right) { | |
| if (($right['rights'] & READ) !== 0) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public static function canGroupsViewDashboard(array $groupIds, $dashboardUuid): bool | |
| { | |
| if (empty($groupIds)) { | |
| return false; | |
| } | |
| /** @var DBmysql $DB */ | |
| global $DB; | |
| $iterator = $DB->request([ | |
| 'FROM' => self::getTable(), | |
| 'WHERE' => [ | |
| 'itemtype' => Group::class, | |
| 'items_id' => $groupIds, | |
| 'dashboard_uuid' => $dashboardUuid, | |
| ], | |
| ]); | |
| foreach ($iterator as $right) { | |
| if (($right['rights'] & READ) !== 0) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| * {@inheritDoc} | ||
| * @see CommonGLPI::getTabNameForItem() | ||
| */ | ||
| public function getTabNameForItem(CommonGLPI $item, $withtemplate = 0) |
There was a problem hiding this comment.
getTabNameForItem(), displayTabContentForItem() and showRightsForm() are all gated behind the UPDATE right only (line 84, 104, 128). PluginMetabaseProfileright (the class this PR explicitly mirrors) gates its tab/form behind READ and only wraps the "Allow/Disallow all" buttons in an extra UPDATE check (inc/profileright.class.php:56,109), so a user with read-only rights on Profile can still see the profile-dashboard matrix. Here, a user with only group/user READ cannot see the tab at all. If this asymmetry is intentional (e.g. group/user READ being too broadly granted to expose dashboard rights read-only), it's worth a one-line comment; otherwise consider splitting the gate like the profile form does.
Co-authored-by: Stanislas <skita@teclib.com>
Co-authored-by: Stanislas <skita@teclib.com>
Update to better manage access to the dashboards.
Checklist before requesting a review
Description
Currently, dashboard access can only be granted per Profile. This PR adds the ability to also grant (or deny) access per Group and per User, using the same rights mechanism (
Profile::dropdownRight) already used for profiles.PluginMetabaseItemrightclass, mirroringPluginMetabaseProfileright, storing rights keyed byitemtype(GrouporUser) +items_id+dashboard_uuid.group/userUPDATE rights, to manage dashboard access for that group/user.I did not add automated tests since the plugin doesn't currently have a test suite in place.
Screenshots: