In past versions of Drupal, you could use _db_rewrite_sql() to get the correct access restrictions alteration of a node query, independent of what you had used as the table alias for the node table. I won't say it was pretty, but it was doable.

In the current state of Drupal 7, it appears to me that http://api.drupal.org/api/function/node_query_node_access_alter/7 is assuming I've used "n" as the alias for the node table. Which means I can't alias it to something else. This is the offending line:

$access_alias = $query->join('node_access', 'na', 'na.nid = n.nid');

This isn't OK, because I have a query that uses the node table more than once (sigh), so I cannot use the same table alias both times (obviously). Besides which, I should be able to alias it however I want to, and the query alter should still work. It's a critical issue for me, because I cannot port my D6 module to D7 without the ability to use my own alias for the node table.

I am not sure what the best way to fix this would be. One idea would be to add meta-data to the query and have this function check to see if there's a "node_table_alias" meta data and use that. That's probably the simplest... It would have to be something added to the query object, obviously.

Another option would be to have the addTag function take a second argument that would give the relevant table alias (or maybe an array of table aliases, to be the most generic possible?), and have an API on the query for the query_alter hook implementations to extract this information.

Thoughts?

I don't see any other query alter functions in core right now...

CommentFileSizeAuthor
#47 701744splittests.patch11.61 KBjhodgdon
#39 701744fixrsstest.patch10.98 KBjhodgdon
#30 701744fixmore.patch10.43 KBjhodgdon
#24 701744withtest.patch8.08 KBjhodgdon
#20 701744withtest.patch8.11 KBjhodgdon
#12 701744working.patch2.32 KBjhodgdon
#10 701744revised.patch2.35 KBjhodgdon
#6 701744.patch2.3 KBjhodgdon
#3 701744.patch1.36 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented

You want addMetaData('base_table_alias', 'n') in node module and replace the hardwired 'n' in the query alter with getMetaData.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Looks like we are on the same page, but what happens if I have multiple node joins in the same query -- how can I make sure they are all altered?

In D6, I would use _db_rewrite_sql() for each piece as I joined it, but...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Active » Needs review
FileSize
701744.patch1.36 KB

Here's a patch that at least should work for a single join, not hard-wiring the "n" table alias. Let's see what the testbot thinks. And reviewers.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

So... If you have multiple joins, it doesn't look like this will work. It would need to find all of the node table aliases, and join a new node_access for each one. If you use sub-query joins, the query alter still only gets run once, in the preExecute step, so it would only be altering the outer query.

That's probably a problem anyway -- if you use db_select() to define a query involving node that needs node_access, and then you tag that query with node_access, it will not trigger hook_query_node_access_alter() to get run, because only the outer query is altered.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

From IRC/Crell:
What about grabbing the table data out of the query, searching it for all node entries, and doing an appropriate join for each of them?
That would then hard-code that hook to filter any instance of the node table, whatever its alias.

My response: Yes, good idea.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
FileSize
701744.patch2.3 KB

Here's a patch that will add node access to all instances of the node table in the query.

At least, I think it should.

Crell’s picture

Crell CreditAttribution: Crell commented

subscribe

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

$query->tables is a protected property. You'll need to use ->getTables() to access that data structure. Remember to assign by reference if you want to be able to manipulate it directly.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Ah, thanks!

I don't think I'd want it by reference, because it would be changed as we march through the tables and do ->join() on the $query object.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
701744revised.patch2.35 KB

Here's a new patch. Hopefully the test bot will wake up and test this one sometime.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

There is an error in the above patch:
foreach ($query->tables

should be
foreach($tables

I am testing this patch and will post another one when I get it working...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
701744working.patch2.32 KB

Here's a new patch. It appears to work, in my tests of the 7.x version of Search by Page (drupal.org/project/search_by_page HEAD), which rely on this functionality working (i.e. being able to join on node with aliases other than "n" and having node access applied).

Maybe the test bot will be working one of these days?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Needs tests

Just a note that we'll need tests for this.

Also subscribing, since I ran into this last night, too. :)

Also, neat trick about ->getTables(). Didn't know about that before.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Do we need a test before the patch can be committed at all?

Also, what's your philosophy on tests -- I mean, should we test node access, or just test that this specific function works on a made-up query and does the right join?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Yes, that's generally the way it works. Without tests, we get regressions, and the less likely we are to hit the bug in normal use, the more critical it is we have tests for it. :)

I'd defer to Larry probably on the best way to test this, but basically we'd add a routine to a test module that's doing a query with a weird alias, and a hook_query_alter() that triggers the original bug. It should fail before the patch is applied, and pass after. I figured this was maybe a more general problem, but it looks from the patch that it's specific to node access?

Crell’s picture

Crell CreditAttribution: Crell commented

What webchick said. :-) The test should setup a situation in the database such that it will pass if the alter hook works on all instances of the node table and fail if it only affects the one with an alias of "n". Then run a query and see what happens.

Something like returning one node if it worked and 2 nodes if it failed and then checking the number of records found, and the id or title of the node that was found, should be sufficient.

All test cases are contrived by definition. :-)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

This bug will make a test crash with an exception, not return more nodes -- because the node access query alter will be trying to join to a table alias of "n" that doesn't exist.

Anyway, I can certainly devise a test in a few lines, and will do so later today when I wake up fully. :)

Crell’s picture

Crell CreditAttribution: Crell commented

To check for that, catch the exception and print the appropriate fail message. See the existing DB tests for examples, since I know we're doing that somewhere already.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Will do, thanks for the suggestion.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
FileSize
701744withtest.patch8.11 KB

OK. Here's a patch with a test, including a new test class.

I have verified that the above patch in #12 on node.module takes the test from failing to passing.

So this patch includes both the test and the patch for node.module from #12.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: -Needs tests

I guess I can take the "needs tests" tag off now?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

Yep! Thanks, jhodgdon!

Just in a quick glance, what's up with this?

+    // Create some content.
+    $node = $this->drupalCreateNode();
+    $node = $this->drupalCreateNode();
+    $node = $this->drupalCreateNode();
+    $node = $this->drupalCreateNode();

You're creating 4 nodes and overwriting the value of $node each time, and then you lose it cos it's in setUp() and not in a protected variable. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I dont' need the value of $node. Guess I shouldn't have saved the return value of drupalCreateNode().

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
701744withtest.patch8.08 KB

Here's a version without saving $node, since it isn't used.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

The changes to the alter hook look good to me on visual inspection.

However, I am not satisfied with the tests here. These are integration tests. That's good for what it's worth, but we also want unit tests. (I'd prefer unit tests to integration tests myself in most cases.) That wouldn't do any HTTP requests at all; just run the exact query on its own in the test method and check the return/catch an exception. Something like:

try {
  $records = db_select('node', 'mytab')
    ->fields('mytab')
    ->addTag('node_access')
    ->execute()
    ->fetchAll();

  $this->assertEqual(count($records), 4, t('Correct number of nodes accessible.'));
}
catch (Exception $e) {
  $this->fail(t('Altered SQL query is malformed.'));
}

(And a similar, second test method for the disallowed user. You can specify the user with the metaData methods of the query object.)

You can leave the integration tests there, but we want to have more "to the metal" tests, too, as there are fewer moving parts.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I did try to do a unit test...

There would have to be more overhaul of http://api.drupal.org/api/function/node_query_node_access_alter/7 for that to work. There are several calls to things that implicitly assume global $user in that function, such as user_access('bypass node access')

Maybe that is a bug in the query alter function, but I'm not sure, since that tag is not particularly documented that I could find, so I wasn't sure about that $account metadata.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Just to be clear: If you run the query in #25, that test will fail because the query will not be altered, because the DrupalWebTestCase leaves the $user as user-1, and that user can bypass all node access checks.

ADDED LATER: I think that is a separate issue about the node_query_node_access_alter, that it implicitly relies on global $user and doesn't fully use the metadata $account. So maybe we should address it on a separate issue report?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Another separate issue with this function is that it implicitly assumes 'op' = 'view' at the top, where it bypasses everything

if (!node_access_view_all_nodes()

If you look at that function, it only checks whether some module(s) are providing grants having to do with view permission, so if the only custom perms are edit/delete, then the whole query alter is bypassed.

So the whole idea of the metadata fields for op and account were not built correctly into this function. But again, I think it's a totally separate issue (or perhaps two) from the issue reported here, which is that the table alias should not be assumed to be "n".

We can't do the test at the low level proposed above with these two issues lingering...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: node_query_node_access_alter will only work if node table alias is "n" » node_query_node_access_alter assumes $op, assumes $user, and will only work if alias is "n"
Assigned: Unassigned » jhodgdon

OK, I'm going to work up another patch that fixes these issues together:
- Need to be able to specify $account
- Need to be able to specify $op
- Need to be able to alias with other than "n".

And have some more tests, more low-level.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
701744fixmore.patch10.43 KB

OK, here it is.

A better-working function, and more tests. This fixes
- Need to be able to specify $account
- Need to be able to specify $op
- Need to be able to alias with other than "n".

And I added to the doc header about those meta data options.

I also reverted just the node.module part of the patch to what's currently committed to D7 HEAD, and the following tests failed:
- Database exceptions when viewing the pages in the browser in testNodeQueryAlterWithUI()
- Using the low-level queries in testNodeQueryAlterLowLevel(), there was no access blocking for the no-access user or the 'update' permission check (both of them could see everything, and the queries weren't altered).

So. Hopefully this will pass muster. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned
cha0s’s picture

cha0s CreditAttribution: cha0s commented

Looks good. Let's see what test bot thinks when he wakes up...

Status: Needs review » Needs work

The last submitted patch, 701744fixmore.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I'll request a retest just in case, and if it still fails "Node RSS Content (NodeRSSContentTestCase)", I'll look into it. I do not think this fix would cause that failure...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review

#30: 701744fixmore.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

This looks like an actual test failure. Investigating.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Grrrrrr.

The problem is that the node_test.module removes all access grants in its node_test_node_grants_alter(), thereby denying all users access to any content that is queried with the 'node_access' tag. So now that the node_access query alter is actually doing what it is supposed to, it is denying access to view content at rss.xml.

Sigh.

I'm going to have to split the test class.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Or better yet, just give 'bypass node access' permissions to the RSS test. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
FileSize
701744fixrsstest.patch10.98 KB

Here's a new patch. All of the node tests pass now. Let's see what the test bot thinks...

Crell’s picture

Crell CreditAttribution: Crell commented

I don't have time to review this at the moment, but just chiming in to say "woot!" on fixing all of these issues. Thanks, jhodgdon!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Do you think we should put a note in the node_test class so that no one else gets bitten by the fact that it actually removes all node access grants, or just leave it?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Actually, I think that maybe we need a separate issue for that, because it's sort of implied that that module is used to test node access, but it would never work correctly, and it references a test that doesn't exist.

I filed #710606: Node access tests are incomplete

cha0s’s picture

cha0s CreditAttribution: cha0s commented

Hey! I looked over this and I must admit I'm not very well versed on the new DB API, so some of it I can't comment on that (eventually, but it'd be better to get someone familiar in here to do it for the mean-time *pokes Crell*)

One thing I did notice though, and I'm not sure if it's a logic bug, or a PHP weirdness, but either way I think we should use parenths around this:

if (!$table instanceof SelectQueryInterface && $table == 'node') {

Like this:

if (!($table instanceof SelectQueryInterface) && $table == 'node') {

I didn't update the patch, cause I'm not sure if I 'get it', so I'll let you take a look first. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

cha0s: That change is fine to make, and does correctly reflect the logic.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

instanceof binds higher than && since it's a boolean test, not a conjunction, so the existing code is valid. I don't think the parens are necessary.

#39 is almost right. The three separate "low level" tests should be in their own test method each. Every test method should be as small and atomic as possible. Can we break that up? Once that's done, this looks RTBC to me.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: Unassigned » jhodgdon

Will do (split tests up), thanks for the review.

Regarding parens and instanceof - I'll put them in, since they also add clarity and will not detract from the code in any way. My philosophy is that if I (or someone else) has to think about precidence, e.g. because a little-used operator is being used, then probably parens are a good idea just to save time when looking at the code down the road.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
701744splittests.patch11.61 KB

Here's a new patch that splits up the tests and adds parens to the instanceof if clause. And incidentally the issue was not about && but about ! for the parens. Clearer with them, IMO, thanks cha0s.

FYI, the reason I didn't split up the tests before is that I've noticed that the setup functions are run again for each test method, so it definitely makes it so the tests take longer to run when splitting up, but obviously I haven't contributed too many core test patches... Anyway, I'm glad to know about the convention and adhere to it (big on standards).

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Ah, I didn't even realize that was the question. Yeah, ! and instanceof can use the parens.

Domo arrigato, Mr. Testboto.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Awesome work on this, jhodgon! Committed to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Removing the call to node_access_view_all_nodes on the 'view' $op is a bug.

See #963656: node_access_view_all_nodes() is never invoked.