Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

ccailly
Copy link
Member

@ccailly ccailly commented Aug 19, 2024

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 ccailly self-assigned this Aug 19, 2024
@cconard96
Copy link
Contributor

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).

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@cconard96
Copy link
Contributor

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.

@cedric-anne
Copy link
Member

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.

@ccailly ccailly force-pushed the fix/crontask-right-preserve-sessions branch from ff5f5cd to 07beb45 Compare September 11, 2024 13:19
@orthagh
Copy link
Contributor

orthagh commented Sep 12, 2024

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

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.

@ccailly ccailly force-pushed the fix/crontask-right-preserve-sessions branch from 07beb45 to 117087f Compare September 12, 2024 12:25
@ccailly ccailly force-pushed the fix/crontask-right-preserve-sessions branch from 117087f to 3692dce Compare September 12, 2024 13:09
@ccailly ccailly requested a review from stonebuzz September 17, 2024 10:29
src/CronTask.php Outdated Show resolved Hide resolved
Co-authored-by: Stanislas <skita@teclib.com>
@trasher trasher merged commit b498d60 into glpi-project:main Sep 23, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants