glpi icon indicating copy to clipboard operation
glpi copied to clipboard

fix: Add all rights to cron and manage session values for CronTask

Open ccailly opened this issue 1 year ago • 2 comments

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !33948

The ticket in question reported a problem with the sending of SLA reminders. The notification was not being generated and fatal errors were interrupting the notification generation mechanism due to undefined variables (glpiname and glpigroups in this case). Access rights to GLPI objects were blocking the notification filter system, which was unable to find the objects due to a lack of rights.

ccailly avatar Aug 19 '24 13:08 ccailly

Maybe time to revisit #14652 to design a more controlled way to handle when we need to selectively ignore permission checks/have more than the user's current permissions.

The method of changing the rights in the session is sketchy because if the crontask is running in the user's session, you could end up with the session being saved with the changed permissions (if the script exits early).

cconard96 avatar Aug 19 '24 13:08 cconard96

If so, would it be safe to use session_write_close() at the start of front/cron.php to prevent this ?

Probably. Although, I don't think it makes sense that the user's session would have been opened to begin with or that a new session would be created.

cconard96 avatar Aug 19 '24 13:08 cconard96

When running cron in glpi mode (where the cron is called using an ajax call to front/cron.php), isn't there a risk that if a fatal error occur the user is stuck with the wrong session data as it would not be restored ?

If so, would it be safe to use session_write_close() at the start of front/cron.php to prevent this ?

The solution is to call ini_set('session.use_cookies', 0); to make sure that the response does not contain any session cookie, and then reinit the session to make sure you use a clean session. However, this kind of "hack" should probably be done at the controller level, not in the Crontask::lauch() method itself.

cedric-anne avatar Sep 09 '24 13:09 cedric-anne

Maybe time to revisit https://github.com/glpi-project/glpi/pull/14652 to design a more controlled way to handle when we need to selectively ignore permission checks/have more than the user's current permissions

for the current need (and issue in !34188), I may be wrong, but I think I'm ok with the current changes.

Anyway, BEFORE the release of 11.0, we need a solution for the workflow plugin @cedric-anne @cconard96 . It's not urgent and can be managed later, but please keep that on your to-do list. We need to work on this plugin later and any barrier should be down before the release.

orthagh avatar Sep 12 '24 07:09 orthagh