[development] Making #access required on forms

[Larry Garfield]

On Fri, 24 Oct 2008 12:00:33 -0400, Angela Byron <drupal-devel at webchick.net> wrote:
> Gerhard brought this up on the security team list, but it seems like
> it's worth broader discussion:
> 
> ---
> 
> Hi there,
> 
> as more an more people use Drupal to provide non-traditional Webpages
> (e.g. providing services using Ajax, Flex, ...) our traditional access
> permission checks in hook_menu are less than ideal.
> 
> For example, you can use drupal_execute to conveniently create content
> or anything else. However, no check for access permissions is done since
> this only happens in the menu hook for node/add/whatever.
> 
> I therefore propose to push for D7 that the #access parameter on forms
> be made mandatory.
> 
> Opinions?
> 
> ---
> 
> My initial thought on the downside is that this has implications for
> people who are using drupal_execute() to perform programmatic tasks
> (node/block/etc. creation, etc.); they would no longer work unless the
> script switched to user with the proper credentials (so we should
> probably get a nice user switching API function in core). It is also
> something in the upgrade steps that, if missed, will cause forms to
> completely disappear which is bound to result in support requests.
> 
> On the other hand, it would provide extra security and would be akin to
> the way we force menu callbacks to provide an access control or they
> don't appear for anyone. It might also help us clean up some nasty
> places in core (node form, I am looking at you) where we have if
> (user_access(...)) hard-coded.
> 
> So, I echo: opinions? :)
> 
> -Angie

I have long argued that any part of a form that should only appear for some users should *always* be added, and then controlled with #access, and the validate and submit handlers must be designed to handle either case regardless of permission settings.  That makes forms much easier to form_alter in useful ways.  I've been bitten by modules not doing that, or not being able to handle a form value that is #access = FALSE, way too many times, especially by OG.  We may need to tweak the internals of FAPI a little so that it handles #access = FALSE in a consistent manner to make the submit hooks easier.

Of course, having to specify #access everywhere would be a nightmare.

So I'd propose:

- *All* FAPI/drupal_render() elements must support #access.  A validate/submit callback that does not function properly with #access set true or false is by definition buggy.

- All elements define whether #access defaults to TRUE or FALSE if not specified.

- For the main form itself, #access defaults to FALSE.  For everything else, it defaults to TRUE.

- Establish a standard to handle conditional form adds via #access, not via an if statement.  Using an if(user_access()) is treated as a bug.  If that causes bugs anywhere, then the bugs get fixed per the first item.

- Document the hell out of the above so that people know the Power of the #Access side.

Does that sound like a reasonable balance?  In the degenerate case for a typical form, it's one extra line to add an #access => TRUE or #access => user_access() to each form.

Of course, one alternative is to have an #access_callback and #access_arguments property pair a la menu API, but I don't know if that's overkill. :-)

--Larry Garfield