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: rename $_SESSION key from 'plugins' to 'glpi_plugins' #282

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

MyvTsv
Copy link
Contributor

@MyvTsv MyvTsv commented Jan 8, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes !35797
  • This RP resolves the issue that the configuration of the Escalation module was saved in the $_SESSION with the 'plugins' key while most other plugins use the 'glpi_plugins' key. This posed an issue with, for example, the ApproveByMail plugin. If the user has rules that modify the group if a validation is accepted with ticket rules and he approves a validation with the ApproveByMail plugin, the old group is not deleted even if the configuration "Delete old groups when adding a new" of climb is set to "Yes".

@MyvTsv MyvTsv marked this pull request as ready for review January 8, 2025 14:31
inc/config.class.php Outdated Show resolved Hide resolved
Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

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

I'm not convinced by this solution, approvalByMail initializes a user session when the user approves a validation request.

    public static function initSession(User $user): void
    {
        static::$old_session_id = Session::getLoginUserID();

        $auth = new Auth();
        $auth->user = $user;
        $auth->auth_succeded = true;

        Session::init($auth);
    }

and the Escalade plugin is designed to work in these conditions (from plugin_init_escalade())

    if ((isset($_SESSION['glpiID']) || isCommandLine()) && Plugin::isPluginActive('escalade')) {

    }

You should check whether the ApprovalByMail plugin initializes the user session correctly

@MyvTsv MyvTsv changed the title fix: Saving configuration outside the $_SESSION fix: rename $_SESSION key from 'plugins' to 'glpi_plugins' Jan 9, 2025
@MyvTsv MyvTsv requested review from Rom1-B and stonebuzz January 9, 2025 10:41
Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

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

Seems good for me,

can you ask the client to confirm that it is working correctly by giving it the patch?

(you'll have to tell it to clean the glpi cache).

@stonebuzz
Copy link
Contributor

and adapt changelog =)

CHANGELOG.md Outdated
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

- Redirect users without ticket rights after escalation.
- Fix private task added when ticket mandatory fields are not filled
- fix: rename $_SESSION key from 'plugins' to 'glpi_plugins'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- fix: rename $_SESSION key from 'plugins' to 'glpi_plugins'
- Ensure plugin works seamlessly in external contexts (e.g., from plugins)

Copy link
Contributor

Choose a reason for hiding this comment

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

In CHANGELOG, the descriptions must be functional, not technical.

@Rom1-B Rom1-B requested a review from stonebuzz January 23, 2025 11:09
@Rom1-B Rom1-B merged commit 0739815 into pluginsGLPI:main Jan 23, 2025
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.

4 participants