Search is not working with node access turned on

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.

Likely a duplicate of #1112854: Subqueries use wrong arguments. Could someone check if #1112854-6: Subqueries use wrong arguments fixes the issue?

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_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 '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

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.

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;

See #3. Can someone check if that patch fixes the problem and makes the queries work?

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

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.

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?

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.

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"?

I reproduced this with both TAC and CA. Steps to reproduce:

1. Grant anonymous users view published content, use search, and use advanced search permissions.
2. Create a node with the following body text: "The bunny's ears were furry."
3. Run cron.
4. Install a node access module and rebuild permissions when prompted.
5. As an administrator, go to search/node.
6. Under advanced search, in the Containing the phrase field, enter bunny's.
7. Search. The node will appear in the search result.
8. Log out. Repeat steps 5-7 as anonymous. The node will not be listed.
9. Disable the node access module. Repeat steps 5-7 as anonymous. The node again shows up in the search results.

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.

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:

function 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.

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.

tested, and it does work for me !
Now i'm trying to figure out why... :)

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.

Tagging.

Can someone see if the patch on #1112854: Subqueries use wrong arguments fixes this issue too?

#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.

I'll try writing a test for this... we'll see how it goes. :)

Here's a test that should fail. It fails locally without #1112854-86: Subqueries use wrong arguments, and passes with that patch applied.

Test rolled with the patch from #1112854-86: Subqueries use wrong arguments; should pass.

Since #1112854: Subqueries use wrong arguments was committed, I believe this becomes a tests-only patch.

Oops. I missed #25 (thanks, xjm!)

Here's that patch again uploaded for testbot's benefit. Let's make sure it passes now.

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?

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.

Rolled with corrections from @jhodgdon's review in #30.

Looks good xjm. Hooray for more tests!

Awesome! Committed and pushed to 8.x and 7.x. Thanks!