Fix(Notifications): attached documents in private followup #21225
Conversation
|
Seems to replace #17544 |
src/NotificationEventMailing.php
Outdated
| $user_id = ($user->getFromDBbyEmail($current->fields['recipient'])) | ||
| ? $user->getID() | ||
| : null; | ||
| $doc_crit = $item->getAssociatedDocumentsCriteria(true, $user_id); |
There was a problem hiding this comment.
Maybe you could use the User object here instead of sending the id?
This would avoid loading the same user twice in a row.
src/User.php
Outdated
| $iterator = $DB->request([ | ||
| 'SELECT' => 'rights', | ||
| 'FROM' => 'glpi_profilerights', | ||
| 'JOIN' => [ | ||
| 'glpi_profiles_users' => [ | ||
| 'ON' => [ | ||
| 'glpi_profilerights' => 'profiles_id', | ||
| 'glpi_profiles_users' => 'profiles_id', | ||
| ], | ||
| ], | ||
| ], | ||
| 'WHERE' => [ | ||
| 'glpi_profilerights.name' => $module, | ||
| 'glpi_profilerights.rights' => ['&', $right], | ||
| 'glpi_profiles_users.users_id' => $this->getID(), | ||
| ], | ||
| ]); | ||
|
|
There was a problem hiding this comment.
One last security concern: when the rights for an user are loaded, we do an additional operation by calling cleanProfile():
Wouldn't something like that be needed here too, to make sure we don't load rights that are out of scope for the profile interface?
There was a problem hiding this comment.
It's just an SQL SELECT, no profile is loaded, so I don't think it's necessary to reload the profile. Or am I misunderstanding your comment?
There was a problem hiding this comment.
I'm thinking about the logic from cleanProfile, not reloadCurrentProfile itself.
Checklist before requesting a review
Please delete options that are not relevant.
Description
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):