Steps to reproduce:

  1. Install D6.19 and TAC 6.x-1.x-dev.
  2. Create a non-required vocabulary.
  3. Create a term in that vocabulary.
  4. Configure TAC as follows for Anonymous:
    Global Default: D/D/D/no/yes
    Default for vocab from #2: D/D/D/no/no
    Term from #3: A/I/I/no/no
  5. Create a node tagged with the term in #3. Anonymous can properly view.
  6. Edit the node and remove the term from #3 so that no terms are selected. Save.
  7. Anonymous can still view the node and the node access records are corrupted.

The reason for this appears to be this bit of code in hook_node_access_records():

  if (is_array($node->taxonomy) AND count($node->taxonomy)) {

For some reason $node->taxonomy is actually an array containing an empty string at this point. So, the statement above matches, despite that there really aren't any terms.

Note that if you don't add the vocabulary default, it actually seems to work.

The difficult-to-reproduce bug in #727696: Global defaults do not work properly on some sites was probably caused by this. #881210: No list or create permissions on any terms may result in incorrect use of default on save is also related (also caused by problems differentiating between the two queries in hook_node_access_records().

A fix would require one of three things:

  1. Checking for this weird empty string format in $node->taxonomy. (Won't solve any other problems and may miss other unexpected forms for $node->taxonomy.)
  2. Add a query to check the actual database records. (Adds overhead, doubling the number of queries hook_node_access_records() has to do for each node.)
  3. Avoid the problem entirely by writing one query that works whether the node has terms or not. (Probably doable; might impact performance of that query; would need to be tested on Postgres as well as MySQL.)
CommentFileSizeAuthor
#6 tac_902858-6.patch2.95 KBxjm
#4 tac_902858.patch2.52 KBxjm
#3 tac_902858.patch2.51 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

This query might work for both cases:

SELECT tadg.rid, 
BIT_OR(COALESCE(ta.grant_view, tad.grant_view, tadg.grant_view)) AS grant_view, 
BIT_OR(COALESCE(ta.grant_update, tad.grant_update, tadg.grant_update)) AS grant_update, 
BIT_OR(COALESCE(ta.grant_delete, tad.grant_delete, tadg.grant_delete)) AS grant_delete
FROM {term_access_defaults} tadg
LEFT JOIN {term_node} tn ON tadg.vid = 0
LEFT JOIN {term_data} t ON t.tid = tn.tid
LEFT JOIN {term_access_defaults} tad ON tad.vid = t.vid AND tad.rid = tadg.rid
LEFT JOIN {term_access} ta ON ta.tid = t.tid
AND ta.rid = tadg.rid
WHERE (tn.vid =%d OR tadg.vid = 0)
GROUP BY tadg.rid
xjm’s picture

Component: Documentation » Code

Uh. Dunno how/why I selected documentation.

Using the test environment described in #903120: Add test suite, I tested the query from #1 and compared its results to those from the current query in hook_node_access_records().

For every possible combination of grants for one, two, and three terms, plus all 256 possible combinations with one term from each vocabulary (4000 or so total), the queries returned the same results. The results were only different when the node was tagged with no terms, in which case the current query returned no results but the query from #1 properly returned the global defaults for each role.

That's enough to make me fairly confident that this query is an acceptable replacement for both queries currently used. This will allow us to avoid weird problems because of unexpected states or structures for $node->taxonomy, and also allows us to avoid the overhead of a second query.

xjm’s picture

Status: Active » Needs review
FileSize
2.51 KB

Confirmed that the new query resolves this bug. Patch attached.

xjm’s picture

FileSize
2.52 KB

#3 has a typo; fixed here:

xjm’s picture

Status: Needs review » Needs work

Does not work properly with multiple nodes, unfortunately.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Alright. I've tried everything I can think of, and the only way I've gotten the correct data is with a subquery or CASE statements. I'd rather not go there, so for now, we will just run the first query, and then run a second query to fetch the global default if the first result was empty. Maybe someone smarter than me can come up with a single query some day so we can reduce the overhead for nodes with no terms, but I'd rather add a few milliseconds of overhead than have bugged node access.

xjm’s picture

I've tested the patch in #6 and confirmed that it resolves both this and the other issue. I also generated multiple nodes tagged with various terms, or none, and all worked properly.

xjm’s picture

double post

xjm’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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