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

NEMA-2900: Change OR Query into OR Condition Group #125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cosmin-flo-tr
Copy link

@cosmin-flo-tr cosmin-flo-tr commented Sep 18, 2020

NEMA-2900

Description

The issue: The SQL query may get extended by other modules (contrib or custom) before getting executed. If the OR conjunction is applied globally, everything that's being extended will be OR-ed. This will surely make the query incorrect.

Our case: group contrib module is extending the queries with its own checks for anonymous users. Instead of constraining the query, it does the opposite because of the OR conjunction.
The result was that even if we have 1 field and 1 entity ID, the query would select all the nodes in the system. Given there are 100k+ nodes, the query would timeout. Again, this happens for guests and, most likely, non-admins.

Change log

  • Changed:
    Moved the global OR conjunction into an OR-ed condition group. This ensures that the OR will get applied to the checked fields only. Any other checks will be added with AND (the default).

@cosmin-flo-tr cosmin-flo-tr changed the title Change OR Query into OR Condition Group NEMA-2900: Change OR Query into OR Condition Group Sep 18, 2020
Copy link
Contributor

@brummbar brummbar left a comment

Choose a reason for hiding this comment

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

Please provide test coverage for this part. Create a test submodule that simulates what the groups module does.

oe_media.module Outdated
@@ -62,10 +62,13 @@ function oe_media_media_access(EntityInterface $entity, string $operation, Accou
}

// Get all the nodes which use this media entity.
$query = $entity_type_manager->getStorage('node')->getQuery('OR');
$query = $entity_type_manager->getStorage('node')->getQuery();
$orGroup = $query->orConditionGroup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name should be snake_case.

@brummbar brummbar added the bug Something isn't working label Sep 21, 2020
@msnassar
Copy link
Contributor

msnassar commented Jun 20, 2024

Hello @brummbar
Due to this query, with group module enabled and configured, unpublished media items that are not being used in any node, are accessible to unauthorized users e.g. Anonymous...

The final query before MR:

SELECT "base_table"."vid" AS "vid", "base_table"."nid" AS "nid"
FROM
{node} "base_table"
LEFT JOIN {node__ucpkn_documents} "node__ucpkn_documents" ON [node__ucpkn_documents].[entity_id] = [base_table].[nid]
LEFT OUTER JOIN {group_relationship_field_data} "gcfd" ON base_table.nid=gcfd.entity_id AND gcfd.plugin_id IN (:plugin_ids_in_use[])
INNER JOIN {node_field_data} "node_field_data" ON base_table.nid=node_field_data.nid
LEFT OUTER JOIN {group_relationship_field_data} "gcfd_2" ON gcfd.gid=gcfd_2.gid AND gcfd_2.plugin_id='group_membership' AND gcfd_2.entity_id=:account_id
WHERE ("node__ucpkn_documents"."ucpkn_documents_target_id" = :db_condition_placeholder_0) **OR** (("gcfd"."entity_id" IS NULL) OR (("node_field_data"."status" = :db_condition_placeholder_1) AND (("gcfd"."type" IN (:db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7)) AND ("gcfd_2"."entity_id" IS NULL))))

The final query after MR:

SELECT "base_table"."vid" AS "vid", "base_table"."nid" AS "nid"
FROM
{node} "base_table"
LEFT JOIN {node__ucpkn_documents} "node__ucpkn_documents" ON [node__ucpkn_documents].[entity_id] = [base_table].[nid]
LEFT OUTER JOIN {group_relationship_field_data} "gcfd" ON base_table.nid=gcfd.entity_id AND gcfd.plugin_id IN (:plugin_ids_in_use[])
INNER JOIN {node_field_data} "node_field_data" ON base_table.nid=node_field_data.nid
LEFT OUTER JOIN {group_relationship_field_data} "gcfd_2" ON gcfd.gid=gcfd_2.gid AND gcfd_2.plugin_id='group_membership' AND gcfd_2.entity_id=:account_id
WHERE ("node__ucpkn_documents"."ucpkn_documents_target_id" = :db_condition_placeholder_0) **AND** (("gcfd"."entity_id" IS NULL) OR (("node_field_data"."status" = :db_condition_placeholder_1) AND (("gcfd"."type" IN (:db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7)) AND ("gcfd_2"."entity_id" IS NULL))))

Notice the diff in both queries (I put it between ** **)

I confirm, the MR proposed by @cosmin-flo-tr is fixing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants