Breaks LoginToboggan

this sounds very odd to me- can you give more details on your setup?

Is the non-authenticated user actually able to add nodes, or juset see the link?

"Product" is a node type you created with the core node module, or is defined by another module?

This sounds very odd to me since admin menu does not touch anything related to permissions or node access, so I'm guessing that there might be some very incorrect code in LoginToboggan

Hmm, ok - well DAM isn't "refreshing" them, but it does its work in the page footer. It's possible that by calling various core access callbacks this "refresh" is happening?

Note, however, that DAM should basically exit and do nothing if users do not have its permission. Also, it really does not do anything during the page load until hook_footer (when all the other content and menus are already rendered), except for the user_access call in hook_init. Where in the page load does Login Toboggan run the code above?

Frankly, I think the code aboves looks dumb - why not just give the absolute minimum of permissions to authenticated users, and then increase the permissions by granting another role (approved users or some such) when appropriate?

/**
 * Implementation of hook_init().
 *
 * We can't move this into admin_menu_footer(), because PHP-only based themes
 * like chameleon load and output scripts and stylesheets in front of
 * theme_closure(), so we ensure Admin menu's styles and scripts are loaded on
 * all pages via hook_init().
 */
function admin_menu_init() {
  if (user_access('access administration menu')) {
    $path = drupal_get_path('module', 'admin_menu');
    drupal_add_css($path .'/admin_menu.css', 'module', 'all', FALSE);
    // Performance: Defer execution.
    drupal_add_js($path .'/admin_menu.js', 'module', 'header', TRUE);

    drupal_add_js(array('admin_menu' => array('margin_top' => variable_get('admin_menu_margin_top', 1))), 'setting');
    if ($_GET['q'] == 'admin/build/modules') {
      drupal_add_js(array('admin_menu' => array('tweak_modules' => variable_get('admin_menu_tweak_modules', 0))), 'setting');
    }
  }
}



/**
 * Implementation of hook_footer().
 *
 * Admin menu was previously output via hook_block(), but suffered from
 * theme-specific stylesheets that may be applied to layout blocks. We now
 * output Admin menu in the footer to circumvent this.
 *
 */
function admin_menu_footer($main = 0) {
  // Check for the flag indicating that we need to rebuild.
  if (variable_get('admin_menu_rebuild_links', FALSE)) {
    module_load_include('inc', 'admin_menu');
    _admin_menu_rebuild_links();
    variable_del('admin_menu_rebuild_links');
  }

  if (!user_access('access administration menu') || admin_menu_suppress(FALSE)) {
    return;
  }

  $content  = '<div id="admin-menu">';
  $content .= admin_menu_tree_output(menu_tree_all_data('admin_menu'));
  $content .= '</div>';
  return $content;
}

Hmm, ok - well DAM isn't "refreshing" them, but it does its work in the page footer. It's possible that by calling various core access callbacks this "refresh" is happening?

it shouldn't, no

Where in the page load does Login Toboggan run the code above?

LT performs it's work both during hook_init, and anytime the 'load' op of hook_user is called. IMO this is pretty good coverage, and if it's being unset there's probably a bad approach somewhere in other code that's killing it.

Frankly, I think the code aboves looks dumb - why not just give the absolute minimum of permissions to authenticated users, and then increase the permissions by granting another role (approved users or some such) when appropriate?

it wasn't a dumb approach when LT was first written. however, after the core decision to remove the auth user from the database, things got trickier. i still think the approach is ok. at this point, it isn't going to change until 7.x at the earliest, so we'll have to live w/ what we have now until then at the very least.

$account is almost always NULL when user_access() is called, so I don't think that's the problem.

I'd look more fro some place that user_access is getting called with the 3rd param ($reset) as TRUE.

The only change I can see here is the one mentioned by pwolanin: DAM shouldn't do anything if the user does not have access to it.

The incompatibility between LT and DAM is actually related to a problem with LT not resetting the permissions cache when it dynamically removes the 'authenticated user' role (thanks to mrtoner and pwolanin for the discussion on user_access that put me on the trail of the real problem). Because DAM calls user_access in its hook_init, if DAM's hook_init is called before LT's, the permissions for the roles set by sess_read (which automatically sets the 'authenticated user' role when a user logs in) will be cached and LT's removal of the 'authenticated user' role will not have the desired effect. I have documented this issue with LT and proposed a patch here: http://drupal.org/node/319438. As I note there, it is also very important that you install a core patch, which fixes a subtle (and nasty) little problem with user_access noted here: http://drupal.org/node/275796 (which also has an attached patch) or else the fix won't work.

Good. So all we can do in admin menu is commit #10, which does not solve this issue, but looks better to me anyway.

@pwolanin: Do you agree?

Committed patch in #10.