Skip to content

Fix KB Access#13303

Closed
tsmr wants to merge 1 commit intoglpi-project:10.0/bugfixesfrom
tsmr:FixKBAccess
Closed

Fix KB Access#13303
tsmr wants to merge 1 commit intoglpi-project:10.0/bugfixesfrom
tsmr:FixKBAccess

Conversation

@tsmr
Copy link
Copy Markdown
Collaborator

@tsmr tsmr commented Nov 15, 2022

In helpdesk interface a user can access to all KB articles

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #

In helpdesk interface a user can access to all KB articles
trasher
trasher previously approved these changes Nov 16, 2022
@AdrienClairembault
Copy link
Copy Markdown
Contributor

AdrienClairembault commented Nov 16, 2022

https://glpi-user-documentation.readthedocs.io/fr/latest/modules/tools/knowledgebase.html

Only public FAQ items are visible to users of simplified interface. Other elements are visible only to technicians via standard interface. FAQ items are public to everyone (within their entity) that has access to read FAQs. The additional visibility restrictions will have no affect in this case.

Are you talking about the last case ? For public FAQ articles, the restrictions on entity/profiles/groups/user are not enforced.

@tsmr
Copy link
Copy Markdown
Collaborator Author

tsmr commented Nov 16, 2022

No, this does not affect public access
In fact when you are not connected: ok for me to see the articles of all the entities from the simplified interface.

But if you are logged in, you will not see all the articles but only those that meet the criteria

@AdrienClairembault
Copy link
Copy Markdown
Contributor

Have you checked the link above ?
The documentation state clearly that any public FAQ ("Put this item in the FAQ") will be visible to all self-service users without restrictions.

@tsmr
Copy link
Copy Markdown
Collaborator Author

tsmr commented Nov 16, 2022

Yes but you have 2 cases :
with or without this parameter :
image

@AdrienClairembault
Copy link
Copy Markdown
Contributor

image

I'm not saying that I agree with it, but that's how its defined.

@cedric-anne
Copy link
Copy Markdown
Member

I have no opinion about the way to filter things, but we recently had many PRs about this. We should probably add some tooltips next to some fields to clarify how it affects visibility. It could help both end-users and developers.

@tsmr
Copy link
Copy Markdown
Collaborator Author

tsmr commented Nov 16, 2022

Yes but I do not agree with this note :(

The targets are here to define who can see the articles (entity, profile, group, user) & I think it should be the same on a simplified interface (except with anonymous access)

When you have several entities (like IT and HR), it is normal, I think that the user having access to the HR entity, only sees the articles of his entity.

@AdrienClairembault
Copy link
Copy Markdown
Contributor

AdrienClairembault commented Nov 16, 2022

Actually, after reading the doc again we may be indeed missing the entity check:

FAQ items are public to everyone (within their entity)

So public articles should in fact enforce entity checks if they are defined (but still ignore profiles/groups/users checks).

Maybe something like this ?

diff --git a/src/KnowbaseItem.php b/src/KnowbaseItem.php
index 404c1195c0..16db20fded 100644
--- a/src/KnowbaseItem.php
+++ b/src/KnowbaseItem.php
@@ -667,7 +667,8 @@ class KnowbaseItem extends CommonDBVisible implements ExtraVisibilityCriteria
     /**
      * Get visibility criteria for articles displayed in the FAQ (seen by
      * helpdesk and anonymous users)
-     * This mean any KB article tagged as 'is_faq' should be displayed
+     * This mean any KB article tagged as 'is_faq' and with valid entity 
+     * restrictions (if defined) should be displayed
      *
      * @return array WHERE clause
      */
@@ -675,10 +676,22 @@ class KnowbaseItem extends CommonDBVisible implements ExtraVisibilityCriteria
     {
         $where = ['is_faq' => 1];

-        // Specific case for anonymous users + multi entities
         if (!Session::getLoginUserID() && Session::isMultiEntitiesMode()) {
+            // Specific case for anonymous users + multi entities
             $where[Entity_KnowbaseItem::getTableField('entities_id')] = 0;
             $where[Entity_KnowbaseItem::getTableField('is_recursive')] = 1;
+        } elseif (Session::getLoginUserID()) {
+            $where['OR'] = [
+                // Items with valid entity restrictions
+                [self::getVisibilityCriteriaKB_Entity()],
+                // Items that have no entity restrictions
+                'NOT' => [
+                    KnowbaseItem::getTableField('id') => new QuerySubQuery([
+                        'SELECT' => 'knowbaseitems_id',
+                        'FROM' => Entity_KnowbaseItem::getTable()
+                    ]),
+                ]
+            ];
         }

         return $where;
(

@trasher trasher dismissed their stale review November 17, 2022 08:24

Under discussionn

@cedric-anne cedric-anne marked this pull request as draft November 18, 2022 11:15
@cedric-anne cedric-anne removed their request for review November 29, 2022 21:42
@cedric-anne
Copy link
Copy Markdown
Member

Closed in favor of #13455.

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