| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | search.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7, Needs issue summary update |
Issue Summary
I've a problem with search.
I would find more than word or word with apostrophe (like owner's). The search work fine if the user is administrator, or has "Bypass content access control". Otherwise is empty.
I debug something and I see a strange situation.
If the user is admin I have:
[arguments:protected] => Array
(
[:db_condition_placeholder_0] => 1
[:db_condition_placeholder_1] => my_content
[:db_condition_placeholder_2] => owner
[:db_condition_placeholder_3] => node
[:db_condition_placeholder_4] => % s %
[:db_condition_placeholder_5] => % owner %
)
and:
[stringVersion] => (n.status = :db_condition_placeholder_0) AND( (n.type = :db_condition_placeholder_1) )AND( (i.word = :db_condition_placeholder_2) )AND (i.type = :db_condition_placeholder_3) AND( (d.data LIKE :db_condition_placeholder_4 ESCAPE '\\') AND (d.data LIKE :db_condition_placeholder_5 ESCAPE '\\') )
but when the user is not admin I have:
[arguments:protected] => Array
(
[:db_condition_placeholder_0] => 1
[:db_condition_placeholder_1] => my_content
[:db_condition_placeholder_2] => owner
[:db_condition_placeholder_3] => node
[:db_condition_placeholder_4] => 0
[:db_condition_placeholder_5] => all
[:db_condition_placeholder_6] => 1
)
and:
[stringVersion] => (n.status = :db_condition_placeholder_0) AND( (n.type = :db_condition_placeholder_1) )AND( (i.word = :db_condition_placeholder_2) )AND (i.type = :db_condition_placeholder_3) AND(( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) ))AND (na.grant_view >= :db_condition_placeholder_6) AND( (d.data LIKE :db_condition_placeholder_4 ESCAPE '\\') AND (d.data LIKE :db_condition_placeholder_5 ESCAPE '\\') ) )
I think that the sencond is wrong because the 4th and 5th position are used to 'LIKE' search . Can anyone help me?
Comments
#1
Yes, this looks like a bug -- it looks like the advanced search is not working at all correctly in Drupal 7/8, and the node access part of the query has overwritten the search matching conditions, for users other than those with "bypass node access" permissions. That seems like a major issue to me, since Search is not working if node access is used.
It's possible that it would also fail with a non-advanced search, but this query looks like it's an advanced search, because the content type is part of the search query.
At this point, we need to investigate and fix this in Drupal 8, then backport the fix to Drupal 7.
#2
I try to disalbe content access module (http://drupal.org/project/content_access), and now search work fine.
#3
Likely a duplicate of #1112854: Subqueries use wrong arguments. Could someone check if #1112854-6: Subqueries use wrong arguments fixes the issue?
#4
Looks like the placeholder in the query are wrong, here the query run for a user non-logged in with node_access enable :
SELECT DISTINCT i.type AS type, i.sid AS sid, SUM((2.3449670046926 * i.score * t.count)) AS calculated_scoreFROM
search_index i
INNER JOIN node n ON n.nid = i.sid
INNER JOIN search_total t ON i.word = t.word
INNER JOIN node_access na ON na.nid = n.nid
INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type
WHERE (n.status = '1') AND( (i.word = 'owner') )AND (i.type = 'node') AND(( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '2') AND (na.realm = 'nodeaccess_rid') )OR( (na.gid = '3') AND (na.realm = 'nodeaccess_uid') )OR( (na.gid = '3') AND (na.realm = 'nodeaccess_author') ))AND (na.grant_view >= '1') AND( (d.data LIKE '0' ESCAPE '\\') AND (d.data LIKE 'all' ESCAPE '\\') )
GROUP BY i.type, i.sid
HAVING (COUNT(*) >= '1')
ORDER BY calculated_score DESC
LIMIT 10 OFFSET 0
#5
The place holders becomes wrong after the query got altered in drupal_alter($hooks, $query) line 1199 of select.inc.
Looks like the query_node_access hook break the query.
#6
I think the query should look like this and the last placeholders has to be changed.
SELECT DISTINCT i.type AS type, i.sid AS sid, SUM((2.3449670046926 * i.score * t.count)) AS calculated_score FROM search_index i INNER JOIN node n ON n.nid = i.sid INNER JOIN search_total t ON i.word = t.word INNER JOIN node_access na ON na.nid = n.nid INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type WHERE (n.status = '1') AND( (i.word = 'owner') )AND (i.type = 'node') AND(( (na.gid = '0') AND (na.realm = 'all') )OR( (na.gid = '2') AND (na.realm = 'nodeaccess_rid') )OR( (na.gid = '3') AND (na.realm = 'nodeaccess_uid') )OR( (na.gid = '3') AND (na.realm = 'nodeaccess_author') ))AND (na.grant_view >= '1') AND( (d.data LIKE '%owner%' ESCAPE '\\')) GROUP BY i.type, i.sid HAVING (COUNT(*) >= '1') ORDER BY calculated_score DESC LIMIT 10 OFFSET 0;#7
julien is right.
the second query (#6) is right and the first is wrong (#5)
#8
See #3. Can someone check if that patch fixes the problem and makes the queries work?
#9
Appied the patch, didn't work for me. To me it looks like the arguments swap during compilation of the where condition.
To check this, check the $queryPlaceholder before compiling and check the $args variable after line 1142 of /includes/database/select.inc
#10
Thanks for testing! Then this issue is *not* a duplicate of that other one. Good to know.
In that case, we need a patch, and preferably a SimpleTest test that fails and demonstrates this bug, and passes with the patch.
#11
With Drupal 7.4 and latest version of content access 7.x dev, it's working for me. Can you please post steps to reproduce?
#12
good_man: In order to trigger node access query mods, you normally have to (a) enable a node access module and (b) configure at least one special node access rule using that module. Did you do both of those, or just enable the module? Also, (c) do the search logged in as not user 1.
#13
Yes I did assigned some nodes grants to user1, and logged in with it, then searched, only words in nodes that user1 has access to appears on simple & advanced search. I'd like to know what do you mean by "one special node access rule"?
#15
I reproduced this with both TAC and CA. Steps to reproduce:
search/node.bunny's.#16
Thanks for the clear steps to reproduce!
There was a node access test module added somewhere along the way with some of the D7 security issues, so we should be able to add tests for this condition.
#17
The same problem happens when using Search Configuration to restrict certain content types from appearing in the search results (see #1238380: Punctuation breaks search for users other than UID 1). If I search for a word with an apostrophe or period, then I get no search results (as a non-admin user). One way you can easily reproduce this issue is to implement hook_query_node_access_alter() and add the following code:
<?phpfunction custom_query_node_access_alter(QueryAlterableInterface $query) {
$node_alias = FALSE;
foreach ($query->getTables() as $alias => $table) {
if ($table['table'] == 'node') {
$node_alias = $table['alias'];
}
}
if ($node_alias) {
$query->condition(db_and()->condition($node_alias . '.type', 'page'));
}
}
?>
What this does is add a condition to the query which restricts the results to only include page content types. Somehow the query ends up searching for the word "page" instead of the actual search terms.
To reproduce this, follow the steps in the issue I referenced above.
It seems to me that the query is using the wrong arguments, which seems to suggest that we might find an answer at #1112854: Subqueries use wrong arguments, though the patch there did not solve the problem for me.
#18
It appears that the conditions used in the SearchQuery class need to be cloned. This one-line patch fixes my issue (above)... please test it against the other scenarios. I think the solution might best be solved with an approach outlined here: #1112854-8: Subqueries use wrong arguments. In the meantime, this patch should solve the search issues.
#19
tested, and it does work for me !
Now i'm trying to figure out why... :)
#20
Without debugging this, I think that this has something to do with the count query. The count query would need to alter the conditions and these would in turn be used incorrectly once the main query is run.
I ran into something similar with search_config module earlier, and worked around it by using a <> condition rather than the NOT IN condition. Reason why one worked and the other didn't was a complete wtf at the time, but maybe this could help someone debug the core issue... This workaround could be potentially used in contrib while the core issue is being fixed.
#21
Tagging.
#22
Can someone see if the patch on #1112854: Subqueries use wrong arguments fixes this issue too?
#23
#1112854-86: Subqueries use wrong arguments does appear to resolve this. Yay! So we just need a test for this to go with that patch.
#24
I'll try writing a test for this... we'll see how it goes. :)
#25
Here's a test that should fail. It fails locally without #1112854-86: Subqueries use wrong arguments, and passes with that patch applied.
#26
The last submitted patch, 1202416-24-test-only.patch, failed testing.
#27
Test rolled with the patch from #1112854-86: Subqueries use wrong arguments; should pass.
#28
Since #1112854: Subqueries use wrong arguments was committed, I believe this becomes a tests-only patch.
#29
Oops. I missed #25 (thanks, xjm!)
Here's that patch again uploaded for testbot's benefit. Let's make sure it passes now.
#30
The test looks good to me. The only thing I would suggest changing is the verb at the beginning of the docblocks -- standard for functions is to start with "Tests" rather than "Test" for one-line descriptions of functions and classes (see http://drupal.org/node/1354). And yes I realize that other tests may not follow the standards, alas!
Hmmm... Also, the tests say they are testing advanced search, but I don't think they are? The only thing passed to search is a keyword, which is not advanced search?
#31
Ah, that's a small point I guess. It searches for a string wrapped in double quotes, which is equivalent to submitting the Containing the phrase field. I'll try to rename/reword it a bit to clarify that.
#32
Rolled with corrections from @jhodgdon's review in #30.
#33
Looks good xjm. Hooray for more tests!
#34
Awesome! Committed and pushed to 8.x and 7.x. Thanks!
#35
Automatically closed -- issue fixed for 2 weeks with no activity.
#36
#37