Skip to content

Conversation

Rom1-B
Copy link
Contributor

@Rom1-B Rom1-B commented Oct 1, 2025

Checklist before requesting a review

Please delete options that are not relevant.

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

Description

  • It fixes !39340

It checks that the recipient user has the necessary permissions to view the documents in private follow-ups before attaching them to the notification.

NB: rework of #17544

Screenshots (if appropriate):

@Rom1-B Rom1-B changed the base branch from 11.0/bugfixes to 10.0/bugfixes October 1, 2025 12:15
@cedric-anne cedric-anne added this to the 10.0.21 milestone Oct 1, 2025
@Rom1-B Rom1-B marked this pull request as ready for review October 3, 2025 09:47
@cconard96
Copy link
Contributor

Seems to replace #17544

Copy link
Contributor

@trasher trasher left a comment

Choose a reason for hiding this comment

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

LTM. Maybe ask for customer validation

@Rom1-B
Copy link
Contributor Author

Rom1-B commented Oct 7, 2025

The customer has just approved (with the latest commit)

src/User.php Outdated
$profile = new Profile();
$profile->getFromDB($profile_id);
$profile->cleanProfile();
if ($profile->haveUserRight($user_id, $module, $right, 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Yous should check the user rights on the entity of the ITIL object. Indeed, the user may not have the right to see private items on the root entity, but have rights to view them in a sub-entity.

Comment on lines 7963 to 7966
if ($user === null) {
$user = new User();
$user->getFromDB(Session::getLoginUserID());
}
Copy link
Member

Choose a reason for hiding this comment

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

Using the $user object instead of the session will add, at least, 5 SQL queries more to display the ticket timeline. It should keep the checks on session when the $user param is null.

if (
    !$bypass_rights
    && (
        ($user === null && !Session::haveRight(ITILFollowup::$rightname, ITILFollowup::SEEPRIVATE))
        || ($user !== null && !$user->hasRight(ITILFollowup::$rightname, ITILFollowup::SEEPRIVATE))
    )
) {

src/User.php Outdated
Comment on lines 7028 to 7029
if ($this->isNewItem()) {
throw new \LogicException('Cannot check rights for a user not yet saved');
Copy link
Member

Choose a reason for hiding this comment

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

Could you check the behaviour with an "external" user? For instance for a ticket with a requester that does not correspond to a valid GLPI user but is targetted only with an email (anonymous tickets from helpdesk).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With an anonymous user, $user is null in getAssociatedDocumentsCriteria(), so do not call hasRight().

Copy link
Member

Choose a reason for hiding this comment

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

Unless I miss something, $user will be an instance of User, but getFromDBbyEmail() will have fail, and I think isNewItem will return true in this case.

$user = new User();
$user->getFromDBbyEmail($current->fields['recipient']);
$doc_crit = $item->getAssociatedDocumentsCriteria(false, $user);

@cedric-anne cedric-anne added the bug label Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants