glpi-project
Feature : targeted private saved searches
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets |
Add option to choose targets for private saved searches. Modify CommonDBVisible to make it possible to restrict the types of target available, and to choose the right used to determine the availability of user type targets. For a saved search, a target can either be a user or a group. User and group dropdown take into account entity of the item when loading available targets.
The class used for the user target might need to be changed (SavedSearch_User is already used so I couldn't follow the naming convention set by the other CommonDBVisible items, so I created SavedSearch_UserTarget for now). Also the new method getVisibilityRight could be used to dynamically get the right for the itemtype associated with the saved search (to only be able to targets users with the read right on the itemtype).
Example :
Target tab for SavedSearch :
Corresponding record in DB :
Available saved searches for the itemtype for users 2 without any target set :
User 2 added as a target :
Available saved searches on the itemtype for user 2 after being added added as a target :
Type of targets available set through the static variable $types :
Restrictions on groups available as targets based on the item's entity (for the example entity set as 1 instead of root) :
List of entities :
List of groups with entity restrictions :
Visibility for groups :
Subvisibility :
@ArthurMinfotel
Could you add some screenshots to your PR description?
I've edited it to add several screenshoots, showing an example and the added functionnalities.
We discussed about it with @orthagh and we think that the
is_privateflag should be removed from the database. Indeed, having both a public/private management + a entity/groups/users visibility management is redundant and can be a bit complex to handle for the end user.At creation, if the saved search is created as public, then it a visibility on the root entity + recursive should be automatically created. If it is created as private, the lack of visibility filters should be sufficient to make it private.
It would require to add a migration to add a new visibility filter to all existing public searches.
The current behavior of the public searches is to have them available for all users within it's entity (and child entities if it is recursive), so shouldn't I rather make it so that it automaticaly create a link with the active entity rather than the root entity if it is meant to be a public search ?
We discussed about it with @orthagh and we think that the
is_privateflag should be removed from the database. Indeed, having both a public/private management + a entity/groups/users visibility management is redundant and can be a bit complex to handle for the end user. At creation, if the saved search is created as public, then it a visibility on the root entity + recursive should be automatically created. If it is created as private, the lack of visibility filters should be sufficient to make it private. It would require to add a migration to add a new visibility filter to all existing public searches.The current behavior of the public searches is to have them available for all users within it's entity (and child entities if it is recursive), so shouldn't I rather make it so that it automaticaly create a link with the active entity rather than the root entity if it is meant to be a public search ?
As we add visibility specs to saved searches, the idea was to consider that a saved search with no visibility is private. In this case, the is_private flag is no more useful.
@cedric-anne Could you tell me if what I've added for the creation of public search matches what you talked about ? Same for the migration. Thanks in advance.
@cedric-anne Could you tell me if what I've added for the creation of public search matches what you talked about ? Same for the migration. Thanks in advance.
It seems to match our functional request, but I do not have time right now to do a complete review.
Made some changes to take the deletion of savedsearch.form.php into account (what was causing issue when I last requested a review, didn't notice the deletion when I merged from main and didn't do any functionnal tests for the minor changes I did after).
I created a new controller to include the action needed for items extending CommonDBVisible : it extends GenericFormController and deal with the 'addvisibility' action.
There are some right issues when loading a saved search with a profile which doesn't have the read right on bookmark_public, which I temporarely fixed by overriding the canView method in SavedSearch to always return true. But I also add to override getMenuContent because of that (which only stop the link from appearing in the menu, the list can still be accessed).
I left it with this temporary solution because I think the rights might get reworked with the deletion of the private property, but if the rights are left unchanged I thought of maybe reworking front/savedsearch.php this way :
If everything is OK from a pratical point of view, I will start working on updating the current test functions and adding a new one for the targets.
Tests are updated. Things that are tested are :
- Automatically adding the saved search's entity as target if input contains is_private with a falsy value.
- For each type of target, visibility of a saved search to a user other than the creator when using SavedSearch::getMine().
It seems like there are still some small lint issues.
For the sonar cloud report, it does not like that you are created an item using a class supplied by the browser:
This has created security issues in the past as malicious users can supply some unwanted class names.
You can fix it by calling getItemForItemtype instead, which will make sure the class is at least a CommonGLPI item which should be safe.
I've updated the controller to fix the issue with the class instance.
For the lint, I believe those are not fixable :
The first and second are called in
getTabNameForItem and displayTabContentForItem, and so lint doesn't recognise the methods from the class CommonDBVisible, which are called when $item is an instance of SavedSearch , since its parameter $item is typed as CommonGLPI.
As the third one, it's a right check, so I don't even know what the cause might be.
I've added suggestions that should fix the typing issues.
For the "always true" issue, this is because the condition exist twice:
So when you reach the second condition, it will always be true because if the first one was false then the function would have returned a value already.
I'll let you review to see which one you need to keep.
I added the suggested fix. I will remember those for future developments.
I rebased the branch to fix conflicts and revert some whitespace changes to reduce the diff. If @orthagh approves the feature, I will do a last technical review and merge it.
I tested the feature:
- with the "glpi" user, I created a savedsearch on the Tickets
- shared it to a "normal" target user
- logged in with the "normal" user
- attempting to access the savedsearch from the Tickets results in a fatal error
By the way, the left menu points to the "Tools" section, instead of remaining in the Tickets section.
Note that if I add the "READ" (Saved Searches) permission for the "normal" user profile (Observer by default), it doesn't change anything.
User can not access the SavedSearch 1
In ./front/savedsearch.php(53)
#0 ./src/Glpi/Controller/LegacyFileLoadController.php(59): require()
#1 ./vendor/symfony/http-kernel/HttpKernel.php(101): Glpi\Controller\LegacyFileLoadController->Glpi\Controller\{closure}()
#2 ./vendor/symfony/http-foundation/StreamedResponse.php(106): Symfony\Component\HttpKernel\HttpKernel::Symfony\Component\HttpKernel\{closure}()
#3 ./vendor/symfony/http-foundation/Response.php(423): Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
#4 ./src/Glpi/Kernel/Kernel.php(266): Symfony\Component\HttpFoundation\Response->send()
#5 ./public/index.php(58): Glpi\Kernel\Kernel->sendResponse()
#6 {main}
