FIX / List widgets showing no results in embed/shared dashboards#24695
FIX / List widgets showing no results in embed/shared dashboards#24695Mary-Clb wants to merge 11 commits into
Conversation
| @@ -1790,7 +1791,13 @@ public static function searchShowList(array $params = []): string | |||
| 'no_sort' => true, | |||
| 'list_limit' => $p['limit'], | |||
| ]); | |||
| Search::showList($p['itemtype'], $params); | |||
| // In embed mode, bypass per-user rights filters so all items in the active | |||
| // entity are visible (entity scoping is still enforced via glpiactiveentities). | |||
| if (Grid::$embed) { | |||
| Session::callAsSystem(fn() => Search::showList($p['itemtype'], $params)); | |||
There was a problem hiding this comment.
Session::callAsSystem() disables all right checks globally via Session::$bypass_right_checks = true, not just the profile-level checks that are missing in the embed session.
Any call to Session::haveRight() returns true for the entire duration of Search::showList(), including item-type and per-item visibility guards.
The admin who shares a dashboard has no way to restrict which items are exposed.
@cedric-anne I'd like to hear your thoughts on this part
There was a problem hiding this comment.
Potential authorization bypass leading to privilege escalation via callAsSystem.
There was a problem hiding this comment.
The itemType and widgetType must be retrieved exclusively from the server-side card configuration (returned by getAllDashboardCards()), and never from HTTP request parameters.
Rom1-B
left a comment
Rom1-B
left a comment
There was a problem hiding this comment.
No test covers searchShowList in embed mode. The callAsSystem branch and the glpilist_limit init are both untested: a test that sets Grid::$embed = true, calls Widget::searchShowList, and asserts the HTML output is non-empty would catch regressions on both.
--> tests/functional/Glpi/Dashboard/WidgetTest.php
| 'status' => Ticket::INCOMING, | ||
| ]); | ||
|
|
||
| // Simulate embed session manually (mirrors what initEmbedSession does, |
There was a problem hiding this comment.
if a new required session variable is added to initEmbedSession later, this test will silently diverge from production behaviour and produce a false positive
Co-authored-by: Stanislas <skita@teclib.com>
…om/Mary-Clb/glpi into fix/embed-dashboard-glpilist-limit
| }; | ||
| $grid->initEmbedSessionForTest(['entities_id' => 0, 'is_recursive' => 1]); | ||
|
|
||
| $this->assertArrayHasKey('glpilist_limit', $_SESSION); |
There was a problem hiding this comment.
assertArrayHasKey('glpilist_limit', $_SESSION) only proves the test fixture's $CFG_GLPI['user_pref_field'] contains glpilist_limit, not that the widget behaves correctly with it. Can you drop it or replace it with an assertion on actual widget output tied to list_limit?
Rom1-B
left a comment
Rom1-B
left a comment
There was a problem hiding this comment.
Several open questions, awaiting a response
3 participants
I have read the CONTRIBUTING document.
I have performed a self-review of my code.
I have added tests that prove my fix is effective or that my feature works.
This change requires a documentation update.
It fixes !44805
When viewing a dashboard through a public share link, list widgets (e.g. ticket lists) were displaying no results at all. The embed session — which is initialized when accessing a shared dashboard without authentication — was missing several variables that the search engine relies on to build its queries correctly: the list limit (
glpilist_limit) and the active user profile rights (glpiactiveprofile). Without them, the search fell back to restrictive defaults that matched nothing. Additionally, the embed mode flag (Grid::$embed) was not set during AJAX card loading requests, preventing the fix from applying at the right moment.This fix ensures the embed session is properly set up with the necessary context so that list widgets can fetch and display items from the configured entity, while keeping data from other entities inaccessible.
Screenshots (if appropriate):