I am calling this major because following the documentation here: http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ac...
would result in a user exposing a page to unauthorized users.
1 would have the "access denied page" and the other would have the form as presented to authorized users.

This problem is mostly identical: http://drupal.org/node/838404

When a page callback of 'drupal_get_form' is used, this solution does not quite work.

When MENU_ACCESS_DENIED is returned from some form, the drupal_prepare_form() function does not properly handle cases where the returned value is not an array().

When MENU_ACCESS_DENIED is returned, no drupal_access_denied() page is presented.

I guess this means that MENU_ACCESS_DENIED does not make it to where it needs to.

Instead I tried calling drupal_access_denied() and returned an empty array().
I get an access denied page and then the form page!

Instead I tried calling drupal_access_denied() and returned MENU_ACCESS_DENIED.

Everything suddenly looks fine, but watchdog tells a different story.

The watchdog now has a lot of messages like:
"Warning: Cannot use a scalar value as an array in drupal_prepare_form() (line 924 of includes/form.inc)."

Looking at the appropriate lines of code, it seems that the drupal_prepare_form() function assumes that $form is an array.

Okay, so I remember telling it to use 'drupal_get_form' as the callback, so I suspect the return statement needs to be handled before it ever calls drupal_prepare_form().

Lets see:
drupal_get_form() calls: drupal_build_form().

Inside of drupal_build_form() at lines 310 and 311 there is:

<?php
    $form = drupal_retrieve_form($form_id, $form_state);
    drupal_prepare_form($form_id, $form, $form_state);
?>

I am pretty sure this is the place to add the check against the $form value.
I tried the following:

<?php
    $form = drupal_retrieve_form($form_id, $form_state);
    if (!is_array($form)) return $form;
    drupal_prepare_form($form_id, $form, $form_state);
?>

And things seem to almost work as expected.
I get the proper error page and I have to remove my explicit drupal_access_denied() call.

However, now the following error appears:
"Warning: Cannot use a scalar value as an array in drupal_retrieve_form() (line 707 of includes/form.inc)."
I suspect that error was already appearing, but it must have been overlooked.

Again we have the same problem, the form value that is returned is not be validated to be a valid array before acting on it:

<?php
  $form = call_user_func_array(isset($callback) ? $callback : $form_id, $args);
  $form['#form_id'] = $form_id;
?>

So I tried the same thing:

<?php
  $form = call_user_func_array(isset($callback) ? $callback : $form_id, $args);
  if (!is_array($form)) return $form;
  $form['#form_id'] = $form_id;
?>

And then everything works fine!

The attached patch makes these changes.

CommentFileSizeAuthor
#1 drupal-7.0-rc1-form_menu_access_denied-1.patch947 bytesthekevinday
drupal-7.0-beta3-form_menu_access_denied-1.patch931 bytesthekevinday

Comments

thekevinday’s picture

thekevinday commented
Version: 7.0-beta3 » 7.0-rc1
StatusFileSize
new drupal-7.0-rc1-form_menu_access_denied-1.patch947 bytes

This affects D7-rc1 as well.

Updated patch has been attached.

damien tournoud’s picture

damien tournoud commented
Status: Active » Closed (won't fix)

Use the API properly: form callbacks should *never* have side effects, especially they should never check for access. Use 'access callback' / 'access arguments' on the menu item for access control.

damien tournoud’s picture

damien tournoud commented

I am calling this major because following the documentation here: http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ac...
would result in a user exposing a page to unauthorized users.

Also, there is nothing wrong with the documentation. The documentation says "Page callback functions wanting to report an "access denied" message should return MENU_ACCESS_DENIED instead of calling drupal_access_denied()", which is perfectly accurate. Form callback functions are not page callback functions.

thekevinday’s picture

thekevinday commented
Title: MENU_ACCESS_DENIED ignroed, page presented twice » MENU_ACCESS_DENIED ignored, page presented twice

The typo was annoying me.

mfer’s picture

mfer commented

FYI, Access denied works fine when the API is properly used. For a simple example take a look at the node add pages. If you don't have access you are told so.

thekevinday’s picture

thekevinday commented

For reference, i was hoping to use drupal_access_denied() when there was a problem ion the form page.
What happens is I might need to do an operation via a hook_form_alter(), such as database loading.
If something goes wrong, I need to reflect that.

I would report the error to watchdog and deny access to the page to the user if the error is not recoverable.
As this is not possible given the problems mentioned above, I have written a function to return my own form page.

Example:

<?php
function page_not_accessible(&$form_state, $id = ''){
  $form = array();

  drupal_set_title("Unable to access page");

  $form['page_not_accessible'] = array(
    '#type' => 'item',
    '#title' => t("Unable to access page."),
    '#description' => t("This page cannot be presented either because there was an error loading the page or you are not authorized to access this page."),
  );

  $form['#id'] = is_string($id) ? $id : '';
  $form['#validate'] = array();
  $form['#submit'] = array();
  $form['#tree'] = FALSE;

  $form_state['process_input'] = FALSE;

  return $form;
}
?>

I can then return this during a hook_form_alter() to prevent the page from being presented in the event of an error.

mfer’s picture

mfer commented

@thekevinday You are explaining your improper use of the Arupal APIs. Instead of explaining your problem and what you think is wrong with Drupal you may be better off asking how to solve your particular problem using the Drupal APIs. You will be less likely to run into problems that way and run into more helpful advice.

Since I am not sure what you want to actually do I can't reapond.

thekevinday’s picture

thekevinday commented

Then there is obvious misunderstanding here.

Here as another attempt to explain:

If module A provides some form B.
Lets say I have a module C than needs to alter form B.
Module C alters form B such that if an error occurs during the building of form B, then form B should not be presented to the user in any manner.
Following the FAPI I return an array().
The submit and validate handlers are removed to help prevent submit and validate from being called as this page should no longer be treated as a form at this point.

This is particularly important when module D is added and that module also alters form B.
Module D might do something to the parts that module C acts on in such a way that it will cause module C to unexpectedly fail while trying to alter form B.
Now that an unexpected case happens, continued execution should be prohibited and the form should not be presented to the user.
A watchdog error message will be logged in the database.
I was merely using drupal_access_denied() as a standard way to stop this.

This prevents the case were two modules conflict and cause compatibility issues.
This is useful for security and data integrity purposes because the form is now in an inconsistent state and I would not assume any callbacks would work properly.

If returning drupal_access_denied() is not allowed by form api, then I follow form api by returning an empty form.
Thus, the example from #6.