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 KB item selection #11923

Closed
wants to merge 1 commit into from
Closed

Conversation

btry
Copy link
Contributor

@btry btry commented Jun 17, 2022

Error while searching for KB items. Regression introduced in a290f76 (file inc/knowbiseitem.class.php)

When searching for KB items from Formcreator, the result may give items without any target (entity, profile, user or group).

The problem is in the 3rd line of the WHERE clause OR `glpi_knowbaseitems`.`is_faq` = '1'

This PR reverts a part of the above mentionned commit

here is the faulty request

SELECT
    `glpi_knowbaseitems`.*,
    `glpi_knowbaseitems_knowbaseitemcategories`.`knowbaseitemcategories_id`,
    GROUP_CONCAT(
        DISTINCT `glpi_knowbaseitemcategories`.`completename`
    ) AS category,
    COUNT(`glpi_knowbaseitems_users`.`id`) + COUNT(`glpi_groups_knowbaseitems`.`id`) + COUNT(`glpi_knowbaseitems_profiles`.`id`) + COUNT(`glpi_entities_knowbaseitems`.`id`) AS `visibility_count`
FROM
    `glpi_knowbaseitems`
    LEFT JOIN `glpi_knowbaseitems_users` ON (
        `glpi_knowbaseitems_users`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_groups_knowbaseitems` ON (
        `glpi_groups_knowbaseitems`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_knowbaseitems_profiles` ON (
        `glpi_knowbaseitems_profiles`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_entities_knowbaseitems` ON (
        `glpi_entities_knowbaseitems`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_knowbaseitems_knowbaseitemcategories` ON (
        `glpi_knowbaseitems_knowbaseitemcategories`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_knowbaseitemcategories` ON (
        `glpi_knowbaseitemcategories`.`id` = `glpi_knowbaseitems_knowbaseitemcategories`.`knowbaseitemcategories_id`
    )
WHERE
    (
        `glpi_knowbaseitems`.`users_id` = '3'
        OR `glpi_knowbaseitems_users`.`users_id` = '3'
        OR `glpi_knowbaseitems`.`is_faq` = '1'
        OR (
            `glpi_groups_knowbaseitems`.`groups_id` IN ('-1')
            AND (
                `glpi_groups_knowbaseitems`.`no_entity_restriction` = '1'
                OR (
                    `glpi_groups_knowbaseitems`.`entities_id` IN (
                        '0'
                    )
                )
            )
        )
        OR (
            `glpi_knowbaseitems_profiles`.`profiles_id` = '1'
            AND (
                `glpi_knowbaseitems_profiles`.`no_entity_restriction` = '1'
                OR (
                    (
                        `glpi_knowbaseitems_profiles`.`entities_id` IN (
                            '0'
                        )
                    )
                )
            )
        )
        OR (
            `glpi_entities_knowbaseitems`.`entities_id` IN (
                '0'
            )
        )
    )
    AND (
        (
            `glpi_knowbaseitems`.`is_faq` = '1'
            OR `glpi_knowbaseitems_users`.`users_id` = '3'
        )
    )
    AND `glpi_knowbaseitems_knowbaseitemcategories`.`knowbaseitemcategories_id` IN ('1', '2', '3', '4')
GROUP BY
    `glpi_knowbaseitems`.`id`

Here is a working requestin GLPI 9.5

SELECT
    `glpi_knowbaseitems`.*,
    `glpi_knowbaseitemcategories`.`completename` AS `category`,
    COUNT(`glpi_knowbaseitems_users`.`id`) + COUNT(`glpi_groups_knowbaseitems`.`id`) + COUNT(`glpi_knowbaseitems_profiles`.`id`) + COUNT(`glpi_entities_knowbaseitems`.`id`) AS `visibility_count`
FROM
    `glpi_knowbaseitems`
    LEFT JOIN `glpi_knowbaseitems_users` ON (
        `glpi_knowbaseitems_users`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_groups_knowbaseitems` ON (
        `glpi_groups_knowbaseitems`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_knowbaseitems_profiles` ON (
        `glpi_knowbaseitems_profiles`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_entities_knowbaseitems` ON (
        `glpi_entities_knowbaseitems`.`knowbaseitems_id` = `glpi_knowbaseitems`.`id`
    )
    LEFT JOIN `glpi_knowbaseitemcategories` ON (
        `glpi_knowbaseitemcategories`.`id` = `glpi_knowbaseitems`.`knowbaseitemcategories_id`
    )
WHERE
    (
        `glpi_knowbaseitems`.`users_id` = '3'
        OR `glpi_knowbaseitems_users`.`users_id` = '3'
        OR (
            `glpi_groups_knowbaseitems`.`groups_id` IN ('-1')
            AND (
                `glpi_groups_knowbaseitems`.`entities_id` < '0'
                OR (
                    `glpi_groups_knowbaseitems`.`entities_id` IN ('0')
                )
            )
        )
        OR (
            `glpi_knowbaseitems_profiles`.`profiles_id` = '1'
            AND (
                `glpi_knowbaseitems_profiles`.`entities_id` < '0'
                OR (
                    (
                        `glpi_knowbaseitems_profiles`.`entities_id` IN ('0')
                    )
                )
            )
        )
        OR (
            `glpi_entities_knowbaseitems`.`entities_id` IN ('0')
        )
    )

    AND `glpi_knowbaseitems`.`is_faq` = '1'
    AND (
        `glpi_knowbaseitems`.`knowbaseitemcategories_id` IN ('0', '1', '2', '3', '4', '5', '6')
    )
GROUP BY
    `glpi_knowbaseitems`.`id`,
    `glpi_knowbaseitemcategories`.`completename`

@btry
Copy link
Contributor Author

btry commented Jun 17, 2022

@Rom1-B : please check if my PR conflicts with the goal you wanted to acheve in your original PR : you said in the PR #9898 Allow access to a knowledge base article by self-service users without it being in the FAQ,

@Rom1-B
Copy link
Contributor

Rom1-B commented Jun 17, 2022

@Rom1-B : please check if my PR conflicts with the goal you wanted to acheve in your original PR : you said in the PR #9898 Allow access to a knowledge base article by self-service users without it being in the FAQ,

This is completely in conflict, as the aim was to explicitly share articles with users (target = user) who do not have rights to the FAQ.

Reverting to the previous code removes this feature.

@btry btry force-pushed the fix_kb_filtering branch from 9d2c7c1 to 4cedd4d Compare June 20, 2022 07:14
@btry
Copy link
Contributor Author

btry commented Jun 28, 2022

@Rom1-B We need to do some extra checks on this PR : If you approved it, I assume it works as you expect in respect of #9898. However, I notice 2 problems

  • a self service user can see KB articles (with is_faq = 0) when the user is granted to view it. But I'm disturbed by the fact that a self service user cannot access a KB article when his group is allowed to view it. I think that such difference of behaviour should be avoided
  • When trying to access a KB item (still is_faq = 0), the self service user will get an "access denied" error. Your PR does not contains any change in right checks, then I believe there is a difference between your use case and the use case I tested. And maybe something to fix un the right checks when opening the KB item.

I add WIP prefix to prevent merging my PR for now.

@btry btry changed the title fix KB item selection WIP: fix KB item selection Jun 28, 2022
@cedric-anne cedric-anne requested a review from Rom1-B July 25, 2022 14:48
@cedric-anne cedric-anne marked this pull request as draft July 25, 2022 14:48
@btry
Copy link
Contributor Author

btry commented Sep 27, 2022

PR has been rebased

@btry btry marked this pull request as ready for review September 27, 2022 06:53
@btry btry changed the title WIP: fix KB item selection Fix KB item selection Sep 27, 2022
@carlosbmc
Copy link

knowbaseitem.class.zip
Hello, I don't find this OR clause in this file.

Thank you!!

@btry
Copy link
Contributor Author

btry commented Oct 6, 2022

Hi @carlosbmc

knowbaseitem.class.php does not exists anymore in GLPI 10. You cannot apply this patch on an older version of GLPI.

@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Oct 7, 2022

To me, it seems more like a functional question.
If Put this item in the FAQ -> Yes means that the item should be visible to everyone then there no bug and this PR should be closed (or maybe there should be a hint in the UI, like disabling the "Targets" tabs with a tooltip explaining why ?).
If this is not the intended behavior then a fix will indeed be needed.

Maybe @orthagh can explain the intended behavior.

@orthagh
Copy link
Contributor

orthagh commented Oct 7, 2022

@AdrienClairembault
Copy link
Contributor

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.

So not a bug for me (I still think the UI could be improved to include this information in one way or another to avoid confusions)

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

See previous comment

@carlosbmc
Copy link

Its correct,
working after removing // OR glpi_knowbaseitems.is_faq = '1'
in file KnowbaseItem.php

@AdrienClairembault
Copy link
Contributor

#12922 should take care of it.

@btry
Copy link
Contributor Author

btry commented Oct 10, 2022

Closing this PR in favor of the above mentionned one.

@carlosbmc : you may want to test that PR as I think it is better to include it in GLPI instead of this one.

@btry btry closed this Oct 10, 2022
@btry btry deleted the fix_kb_filtering branch January 3, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants