Differentiating between 404 and access denied errors

Are you sure anonymous users have the 'access content' permission set?

This is a bug along the same lines as the one reported here. When I disable content viewing for anonnymous users the home page gives out a blank page/page not found/error 500 instead of the "Access denied/You are not authorized to access this page." notice from Drupal 4.3.2. Allowing content viewing makes the front page load as normal. Is there a way to make Drupal 4.4.x CVS act similarly to 4.3.2 when content viewing is disabled?

Here is a potential patch for this problem.

mathias, that patch worked like a charm.

That is not a good solution, IMO.

After digging a little deeper, this problem seems to be a real can of worms. It's not going to be easy for Drupal to differentiate between content that an user is not allowed to view versus content that doesn't exist.

Currently each modules' link hook is responsible for deciding what the user can and can't access. We've seen it a hundred times:

if (user_access('access content')) {
  menu('node', t('content'), 'node_page', 0, MENU_HIDE);
}

I think this is wrong place for this logic since there is no feedback mechanism.
All we know is that the path an user is trying to access isn't in our menu tree. But we have no idea why it's not there.

The only solution I have at this point is to pass along the user_access permission to menu. So the above example would become:

  menu('node', t('content'), 'node_page', 0, MENU_HIDE, 'access_content');

Then when menu evaluates the active URI, it can return success or the type of error it encountered (404, unauthorized access, etc).

What do you think? Does this sound like the right direction?

that mathias solution looks nice ... an alternative would be for omdules not to do access checking around the menu() calls. instead, do access control and error reporting in the callback - i.e._page()

The check could be a lot more complex then just a user_access() call, that is the problem... Menu items might appear depending on where you are (the watchdog items are implemented this way). Or menu items might depend on admin settings (a pending patch for weblinks.module does just that).

Moshe, that alternative would add a lot of menu items which users will only get an 'access denied' page for. Even anonymous users would see the 'administer' link... Would be funny...

Here is a patch for CVS that shows my current approach.

The menu() function now takes an additional paramater which is a permission ('access content', 'administer stories', etc) to pass on to user_access. From this the menu system builds two components: the valid menu tree structure and a list of paths that the user is not allowed to access.

If a permission is not passed into menu(), it is assumed the user has access rights.

The new approach will allow the menu system to differentiate between a 404 versus access restrictions.

I consider this patch a draft and upon approval, will submit a revision that is documented.

Wouldn't it be easier to just provide a MENU_DENIED constant for this? It would probably be faster, because when you deny access to an entire menu subtree (e.g. admin), you only need to register the root item ('admin') as denied, and automatically all subpages will be denied as well. At least that's the way I understand how the menu system works.

I'm in favor of the MENU_DENIED approach if that would be a viable solution. The reason I like that better is because the module adding the menu item can perform _any_ check around the call to menu(). Hence, we are not limited to user_access().

I'm in favor of MENU_DENIED, however the access checking for menu items and in the _page callbacks used in most core modules result in a lot of duped logic. It would be nice if we started deprecating the _page hooks where possible and just add serveral callbacks. I've done this with most of my recent modules and it works fine. Also centralizes most of the modules access checking with menus (if you can't access the menu you cant access the callback). There are only some cases with very complex access mechanisms that this doesn't work.

Once the next big round of menu system enhancements gets committed (which include admin-customizable menus), I'll tackle this using the MENU_DENIED approach. At the same time, I will look to reduce duplicated logic per Kjartan's suggestion; I too prefer the multiple-function approach and have used it successfully in modules. Assigning this to myself.

Here's the next round of menu system enhancements, which address this bug in particular. The MENU_DENIED approach was followed. As a side benefit, administrators will be able to define a custom 403 page, just as they can define 404 pages now. Breadcrumbs should be cleaned up in some cases as well.

Following this patch, some work should be done in ensuring that the new drupal_access_denied() function is used for 403s thrown by modules. Currently, message_access() is presented to the user in a variety of ways; some in drupal_set_message() calls, some as the text of a page, and so forth.

Attaching the patch would help.

Committed to HEAD. Thanks again, JonBob!

In an ideal word, we would soon be able to remove message_access().