6.x-1.7 has caused display of acidfree albums to fail for everyone except the superuser.

This is happening because of the change in forum_access_init() where the following lines were added:

 if (!forum_access_access($tid, 'view')) {
        drupal_access_denied();
        module_invoke_all('exit');
        exit;
      }

Right before that, $tid was assigned the value returned from _forum_access_get_tid($node) and this function is implemented as

/**
 * Return the forum tid or FALSE.
 */
function _forum_access_get_tid($node) {
  return (isset($node->forum_tid) ? $node->forum_tid : (isset($node->tid) ? $node->tid : FALSE));
}

I have no idea what forum_access uses $node->tid for but acidfree sets this with the term id that is assigns for each album. Ideally, _forum_access_get_tid should only return $node->tid if its actually a forum term id.

I have worked around this by checking the $node->type in forum_access_get_tid($node)

function _forum_access_get_tid($node) {
  // kjh:
  if ($node->type == "acidfree")
    return FALSE;
    
  return (isset($node->forum_tid) ? $node->forum_tid : (isset($node->tid) ? $node->tid : FALSE));
}

This is not really a fix because it is conceivable that there are other modules setting $node->tid but it least it makes my albums viewable.

Comments

salvis’s picture

salvis
he/his
commented
Status: Active » Needs work

Yes, that's clearly a bug that we need to fix. Thank you for your report!

salvis’s picture

salvis
he/his
commented
Status: Needs work » Closed (works as designed)

I have to revise my initial assessment.

it is conceivable that there are other modules setting $node->tid

No, not really. $node->tid is used by core forum.module, and contribs should not try to "reuse" core properties.

Checking the vid of the $node->tid would require a database lookup. Making everyone pay to accommodate a misbehaving contrib is not the right solution.

Acidfree should call its property $node->acidfree_tid and there'll never be a conflict again.

mwheinz’s picture

mwheinz commented
Status: Closed (works as designed) » Needs review

Tid is not a special field for just forums, it is used by all nodes. It's the term identifier used by anything that uses vocabularies and taxonomy.

The bug here is that forum access acts as if tid only applies to forums. This is not true. Changing acidfree to use a different field would break taxonomy and most of the rest of drupal.

Please see http://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/6 for reference.

salvis’s picture

salvis
he/his
commented

Please see http://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/6 for reference.

Where exactly are you trying to point me to? Please cite the text.

salvis’s picture

salvis
he/his
commented
Status: Needs review » Postponed (maintainer needs more info)

Tid is not a special field for just forums, it is used by all nodes. It's the term identifier used by anything that uses vocabularies and taxonomy.

It's correct that tid is present in all nodes, but core does not use it for anything but forums.

The rest of the statement is wrong. Taxonomy is rarely limited to a single term and thus tid is useless for taxonomy.

mwheinz’s picture

mwheinz commented

Then why do taxonomy-based access controls like TAC and TAC-Lite work correctly with AcidFree?

As the reference I showed you indicates, tid is used by taxonomy. It is not intended to be "forums only".

At a quick grep through the source shows, tid is referenced in several core modules, as well as views and the aforementioned tac_lite - which works correctly with acidfree.

Do you have some reference that specifies that tid should not be used by 3rd party modules?

salvis’s picture

salvis
he/his
commented

Please answer the question in #4.

salvis’s picture

salvis
he/his
commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

No follow-up.

jvieille’s picture

jvieille commented
Issue summary: View changes

My fix : check if $tid is in forum table before evaluating access. We are not allowed to care about tids that are not in our realm.

function forum_access_access($tid, $type, $account = NULL, $administer_nodes_sees_everything = TRUE) {
+ if (db_result(db_query('SELECT * FROM {forum} WHERE tid = %d', $tid)) == FALSE){
+ return TRUE;
+ }

static $cache = array();
if (!$account) {
global $user;
$account = $user;
}

salvis’s picture

salvis
he/his
commented

What version are you refering to, jvieille? Still D6?

jvieille’s picture

jvieille commented

Yes, D6 / Pressflow